-
-
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 597: Apply grammar, syntax and polish fixes, and clarify phrasing and terminology #1887
Conversation
775c36f
to
ec69c53
Compare
Thanks for the great feedback @methane ! I replied to you with updated suggestions to reflect your requests, and I'll apply them in one go once you agree with everything (I forgot this was my PR and not yours, sorry!) |
Co-authored-by: Inada Naoki <[email protected]>
Thanks for all the help and great feedback, @methane ! I applied all the suggestions, so looks like this is ready for you to review. Let me know if there's anything else that needs fixing! |
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 Thanks @CAM-Gerlach and @methane
Hello @methane ! Thanks so much for all your hard work on this and congratulations on the acceptance of PEP 597; I've personally been very much looking forward to this change, as I have run into issues with other libraries, tools and applications not explicitly specifying their encoding (particularly in the scientific Python realm) and breaking on Windows or other non-UTF-8 platforms countless times over the years, including just a few days ago in regebro/pyroma#28 .
Since I moonlight as a technical writer and copyeditor (and docs, theme and website maintainer for the Spyder IDE), I had noticed a number of minor grammar/textual issues with the prose, so thanks to @JelleZijlstra 's kind encouragement on @corona10 's PR #1885 that accepted this, I went ahead with suggesting some textual fixes and clarifications.
I've broken down the change into several commits to make it easier to review:
encoding
parameter, the arguments to it, and the-X warn_default_encoding
option. Previously, option was inaccurately used to refer to all three, sometimes even within the same sentence, creating significant potential for confusion and not following the correct meaning. While I did attempt to be consistent, in some cases where the meaning was clear I sacrificed absolute rigor to avoid excessive verbosity, but I'm happy to revise that.encoding
parameter is to in the abstract, given the number of different places that use encodinglocale
argument there as well, to silence the warningopen()
without anencoding
argument, and add a parenthetical mentioning that future versions of Python could potentially modify the default, making it even more important to be explicitutf-8
might be made the default in the future (as there are many other places a default encoding is used, some UTF-8 and some platform-specific)locale
argument with regards to Python 3.10 compatibilityencoding_warning
flagEncodingWarning
is only shown by default if theencoding_warning
flag is set, as this is otherwise very confusingtext_encoding
function, and make the latter more descriptive and provide basic instructions on how to respond to it; if the latter is not desired, everything after the semicolon;
can be removed, which will restore the previous text with just fixes to the grammar and terminology. I will also take a look at bpo-43510: Implement PEP 597 opt-in EncodingWarning. cpython#19481 if similar changes are desired there.I will also leave a few comments on things that might need particular attention from you. Looking forward to hearing what you think, and let me know if you have any questions about anything. Thanks again!