-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
Grammar fixes to whatsnew/3.8.rst #16621
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
@akuchling, This should be merged into master first and then backported to 3.8 as the 3.8.rst exists both places. (or forward ported from here to master but you'd have to do that manually with cherry-pick) |
(See related bpo-38450.) |
When you're done making the requested changes, leave the comment: |
I suggested a 4 minor changes but this is otherwise an essential improvement that should be merged before 3.8.0 final even without them. |
This needs to be applied to 3.9, then backported to 3.8. Otherwise, it messes-up our workflow and the other PR that work on 3.8. |
4825b93
to
7294545
Compare
I have made the requested changes; please review again. (I also changed the PR to be against master and not 3.8.) |
Thanks for making the requested changes! @rhettinger, @terryjreedy: please review the changes made to this pull request. |
I would not try to push to another branch in the existing pull request because you end up with a merge nightmare. I would just open a separate PR for master and copy the files from your 3.8 PR there. |
This all looks fine to me. Let's go ahead and get this in, so I can continue to make additions and improvements. |
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.
only minor comments, the can be addressed later if you want to merge this as is.
Doc/whatsnew/3.8.rst
Outdated
@@ -1202,14 +1185,14 @@ Optimizations | |||
+26% on Linux, +50% on macOS and +40% on Windows. Also, much less CPU cycles | |||
are consumed. | |||
See :ref:`shutil-platform-dependent-efficient-copy-operations` section. | |||
(Contributed by Giampaolo Rodola' in :issue:`33671`.) | |||
(Contributed by Giampaolo Rodola in :issue:`33671`.) |
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.
The best spelling is actually Rolodà. Can we use Unicode for names at least?
Even if I've rebased the branch to have just my changes atop master? I'd expect the PR should be fine then, no? |
For one thing, all those code-owner reviewers were spuriously added to the PR. There's nothing to be done about that now. In this case, it will probably be OK since it is only one file and we automatically do squash merges but I've found it is best to avoid changing branches with our workflow. |
The tradeoff is 5-10 minutes to make a fresh branch and PR from freshly updated master versus the possibility that committing this will cause some problems. I would go with the former. (I am literally on my way to bed. Otherwise, I would offer to do it.) Either way, commit so that Raymond can do a pass over the updated file. |
Closing in favour of an updated PR against master: #16745 |
I read through the 3.8 document and fixed various grammar issues. Also, the library modules weren't quite in alphabetical order, and there was a duplicated idlelib/IDLE section (maybe a merging mishap?), so I rearranged them without making any other textual change in that commit. This means the PR is likely easier to read if you go commit-by-commit instead of reading the entire diff.