-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 594: Apply typofixes & language tweaks #2334
Conversation
Just a few things I noticed on my read-through.
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.
A few minor further tweaks, otherwise LGTM. Thanks @bskinn !
Adds two missing commas and rewords the 'these packages are no longer slated for deprecation' language -- thanks, @CAM-Gerlach! Co-authored-by: CAM Gerlach <[email protected]>
The Post-History at the top needs updating too:
What is/are the new dates? It was posted to Discourse on 4th Feb: https://discuss.python.org/t/pep-594-take-2-removing-dead-batteries-from-the-standard-library/13508 (We probably need to include Discourse in PEP 1 too.) |
@hugovk Would that be better fixed in a separate PR? I had opened this one intending it not to change anything substantial. |
- Harmonize header capitalization - Add ``parser`` code formatting - Add whitespacelines under "Update #" headers for consistency.
See #2266 ; a PR to implement what was discussed there is pending merge of #2302 , to avoid the chance of conflicts
I'd consider that a pretty minor change, but I agree its out of scope here in a copyediting/proofreading PR. The discussion link should be updated as well; I can do that in a separate PR. |
Agreed that it's minor, but it seems like choosing the correct date to put there may take some discussion, which would probably be better sited with/near related discussion. |
PR opened as #2335 to update the Discourse link and post date
I'm not sure how it is ambiguous, given its just the date that the new Discourse thread was posted (and not terribly critical regardless), but I do agree it is not really in scope here and would be better handled in a separate PR, which I've opened as above. |
Had just never noticed that the |
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.
LGTM now, thanks @bskinn !
@brettcannon I assume you'll want to look this over and merge, since this is your PEP? |
@CAM-Gerlach 🤷 there's no semantic change here, so no need to wait on me specifically. |
@bskinn thanks for the PR! |
Just a few things I noticed on my read-through.