Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-125665: Update turtledemo docstrings with correct file names#125691
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Wulian233 commented Oct 18, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
terryjreedy commented Oct 20, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
As explained in #118673 (comment), backports will have merge conflicts, like #123457 did. So fixing more than the 3 header lines now is likely a good idea. I believe that if you have the diff in a file, you can just add EDIT: Cancel this: "We would have to check all examples in 3.13 to make sure they have the same exact |
terryjreedy commented Oct 20, 2024
I decided to skip news as there is no reason I can think of that anyone should be hinted to possibly read the revised docstring. |
Wulian233 commented Oct 21, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Sorry I didn't understand what should I do :( . Do I need to open another PR of 3.13/3.12 to resolve the conflict after merge? The above suggestion has been resolved, thanks edit: I have made the requested changes; please review again |
terryjreedy commented Oct 22, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I unresolved comments 1 and 5 (though gh falsely still claims outdated). Fix 5 first. I believe 1 can be done with sed, which comes with Git for Windows. Start menu => Git => Git Bash starts a Linux-like terminal that recognizes sed (stream editor) commands. I used it several years ago after finding and reading a sed manual. I will do so if need be. (It might be easier to first undo the change to (edit) colormixer.py in the local branch version -- I don't know if sed on multiple files stops on error or merely reports and continues with the next one.) When I merge to main, I will try backporting to be sure that the merge conflict for clock.py was due to (or remains after) changing the docstring rather than only resulting from deleting the coding line. Assuming the backport fails, the first step will be to switch to a 3.13 branch (or workspace) and copy the new main turtledemo files into the turtledemo directory of a 3.13 PR branch. I may ask other core developers if we need to add back the #! line. If so, sed would be easiest. I will let you try if you wish or refresh my sed-fu. |
terryjreedy commented Oct 23, 2024
Looks good as of my last comments. Tomorrow I will recheck. Since there is likely to be a backport problem, I will also expand comments to see if I want to make any further changes now before merging. |
Uh oh!
There was an error while loading. Please reload this page.
terryjreedy commented Oct 23, 2024
I added periods to 3 files. Any further changes past line 3 will backport properly. |
Thanks @Wulian233 for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, @Wulian233 and @terryjreedy, I could not cleanly backport this to |
terryjreedy commented Oct 23, 2024
Of the 19 examples, 13 have shebang lines an have the executable mode bit; the other 6 have neither. Using cherry_picker showed that the 13 have merge conflicts because of the shebang lines. I think we should leave shebang line presence and file mode alone. I am thinking about how to be best do this while changing the docstring. |
terryjreedy commented Oct 23, 2024
Thank you for the main PR. I can think of 3 backport approaches, and all seem more trouble than the result is worth. I am now inclined to skip backports, close the issue, and move on to other isssues. |
Wulian233 commented Oct 24, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
If you'd like, I can create a manual target of 3.13 Should I keep or remove shebang? Retaining shebang will still be a conflict in future backports, will removing shebang be considered a new feature and not accept 3.13 backport? |
terryjreedy commented Oct 24, 2024
At least part of the reason that shebang/mode changes were not backported was compatibility considerations (https://peps.python.org/pep-0387/). (IDLE exception. ) As I said above, "we should leave shebang line presence and file mode alone". This multiplies the time needed for a [3.13] backport by at least 2. If you produce one ready-to-merge for fun/instruction, I will merge it, but I prefer to work on other issues. |
…python#125691) Co-authored-by: Wulian <[email protected]> Co-authored-by: Terry Jan Reedy <[email protected]>
edit: skip news I think
📚 Documentation preview 📚: https://cpython-previews--125691.org.readthedocs.build/