Skip to content
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

Merged
merged 6 commits into from
Apr 4, 2021

Conversation

CAM-Gerlach
Copy link
Member

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:

  • The first commit just fixes unambiguous grammar and syntax errors, mostly missing articles ("a"/"the"), phrasing issues, incomplete sentences and minor formatting tweaks, so it shouldn't require much review
  • The second commit is a light copyediting pass on the prose, focused on making the existing text more clear, idiomatic, polished and readable, cleaning up awkward phrasing, choppy flow, repeated words and potentially ambiguous syntax; while I made an effort to avoid any changes that might impact meaning in this pass, you should you're happy with it
  • In the third commit, I apply clearer and more consistent terminology for the 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.
  • In the forth commit, I added several miscellaneous clarifications throughout the text that went beyond the restrictive scope of the second commit; in particular:
    • Mention what function the encoding parameter is to in the abstract, given the number of different places that use encoding
    • Mention the primary purpose of the new locale argument there as well, to silence the warning
    • Clarify that the install error occurs in any non-UTF-8 locale, not just ASCII (which is in fact not the default for any locale on Windows; in fact, nothing guarantees that the default locale encoding on an OS is even ASCII-compatible)
    • Be more explicit about the potential assumed meanings for open() without an encoding argument, and add a parenthetical mentioning that future versions of Python could potentially modify the default, making it even more important to be explicit
    • Specify the specific purpose (opening files) for which utf-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)
    • Try to clarify what is meant in the rationale for the locale argument with regards to Python 3.10 compatibility
    • Be explicit about the value of the encoding_warning flag
    • Clarify that the EncodingWarning is only shown by default if the encoding_warning flag is set, as this is otherwise very confusing
  • In the final commit, I apply the relevant previous changes to the docstring and warning text for the text_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!

pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member Author

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!)

pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
pep-0597.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member Author

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!

Copy link
Contributor

@willingc willingc left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants