-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-85792: Update shebang to python3 #130863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
I checked the devguide, step8 said Add a News entry into the Misc/NEWS.d directory as individual file. But I checked dir Misc/NEWS.d/ , seems like not all fixes have add a News entry. I am not sure if this small fix need it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this file, it would be better to remove the shebang and executable bit.
(Ref previous PR and ticket)
The linked ticket is closed. Before opening PRs, could you comment in tickets first? (In the Python project, we like to discuss should we do something in an issue or on the forum first, then the PR and its reviews can focus on how to do it. But first we agree about the need for the thing.) |
According to PEP 394, python may or may not be installed depending on a distribution or system configuration, so update the shebang to python3 [1] https://peps.python.org/pep-0394/ Signed-off-by: Changqing Li <[email protected]>
Please read the devguide and avoid force pushes – thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the 2 WASM script updates make sense to me. The others make sense as well on the face of it, but I'll leave it up to those domain experts to verify.
Hi,
Since I already force pushed for this pull request, do I need to close this one, and create another pull request? |
You don't need to "fix" the old commit - we want to see the process of getting to the final result reflected in the commit history. So - you make a new commit, on top of previous commits, and push the update to the PR's branch. No If/when the PR is accepted, Python uses merge commits to merge the PR - so even if your development process required 20 tiny back-and-forth commits to get all the details right, this PR will result in a single commit on Python's main branch. |
Thanks, got it. I thought the community need a clean history before. Since I already forced push for last comments fix, I cannot get them back, so I will do nothing now, if there is more comments later, I will follow up the guide. |
@merwok kindly ping, anything else need me to do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change useful? Are there people invoking Parser/asdl_c.py
like this, or is it the makefile (and other build systems) running something like $PYTHON_FOR_BUILD $srcdir/Parser/asdl_c.py
?
I don't think this PR is useful: developers have managed without it for several years. We might accept a PR that just removed shebangs from the files in A |
I agree with Adam. Thanks for the PR regardless! If you decide to create a smaller PR, please create a new ticket: the other one is about tools and already closed as solved in 3.10. |
According to PEP 394, python may or may not be installed depending on a distribution or system configuration, so update the shebang to python3
[1] https://peps.python.org/pep-0394/