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 282: Improve upon colloquial phrasing #2667

Merged
merged 4 commits into from
Aug 3, 2022
Merged

Conversation

Kebap
Copy link
Contributor

@Kebap Kebap commented Jun 24, 2022

Syntax highligting interprets ' as start of a string and colors the following text differently.
Also this change makes the text fragment better readable for non-English native speakers.

Also it is better readable for non-English native speakers.
@Kebap Kebap requested a review from vsajip as a code owner June 24, 2022 13:02
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jun 24, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@JelleZijlstra
Copy link
Member

We generally avoid unnecessary changes to old PEPs, so I'd rather keep the previous wording here.

It's true though that the syntax highlighting is wrong in the second code block: https://peps.python.org/pep-0282/#simple-example. We should fix that so it doesn't get formatted as if it's Python code.

@CAM-Gerlach
Copy link
Member

Yeah, the real fix here is replacing the non-Python :: literal blocks with more appropriate constructs:

  • .. code-block:: text in the second code block that @JelleZijlstra mentions
  • The first one could be converted to separate code blocks with the correct highlighting mode on each (:: or the equivalent .. code-block:: python on the first two sub blocks, and .. code-block:: console on the third, with % replaced by $)
  • The third and fifth code blocks should be bulleted lists instead as they aren't actually code/literal blocks at all.

This really should be done consistently for older PEPs that use non-Python syntax in code blocks so they render as intended, which in many/most cases can be done by adding .. highlight:: <LEXER> at the top of the file, but I just haven't gotten to it yet.

You're welcome to implement that here if you'd like, otherwise I'd be happy to submit a replacement PR to take care of it. Just let us know!

@Kebap
Copy link
Contributor Author

Kebap commented Aug 2, 2022

keep the previous wording

Done.

.. code-block:: text in the second code block that @JelleZijlstra mentions

Done. Not sure if it works as expected. Any way for me to review the results before comitting?

The first one could be converted to separate code blocks

Done. Did not change contents either, so now there are three adjacent code blocks.

The third and fifth code blocks should be bulleted lists instead

Done.

adding .. highlight:: <LEXER> at the top of the file

Did not understand. Did not touch any other PEP files, as it seems out of scope here.

CRITICAL
* DEBUG
* INFO
* WARN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the PEP was written, WARN was replaced by WARNING as preferable.though WARN remains as an alias. Perhaps this should be reflected here too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can defer to your judgement as PEP author, but per PEP 1,

In general, PEPs are no longer substantially modified after they have reached the Accepted, Final, Rejected or Superseded state. Once resolution is reached, a PEP is considered a historical document rather than a living specification. Formal documentation of the expected behavior should be maintained elsewhere, such as the Language Reference for core features, the Library Reference for standard library modules or the PyPA Specifications for packaging.

Presumably there are other things that are out of date here too, so IMO there isn't a need to update this just because it happened to be touched in a syntax update to ensure the PEP renders as intended.

I just merged #2702 which adds a new banner to point readers to where they can find the up to date, canonical documentation for the feature proposed by the PEP once its Final, which we could use to add a prominent link directing readers to the stdlib logging module reference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

pep-0282.txt Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Aug 3, 2022

Looks like this is generally good to go from a PEP editor perspective on my end, with one comment. Thanks!

Done. Not sure if it works as expected. Any way for me to review the results before comitting?

As mentioned in the README, you can both check the RTD rendered preview on the PR, or build the PEPs yourself locally.

Did not understand. Did not touch any other PEP files, as it seems out of scope here.

Sorry, I realize that was confusing—that was not intended to be anything you needed to do.

Co-authored-by: CAM Gerlach <[email protected]>
Copy link
Member

@CAM-Gerlach CAM-Gerlach 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 @Kebap and @vsajip

@vsajip , as PEP author I'll leave it to your discretion to merge

@vsajip vsajip merged commit b9a70c2 into python:main Aug 3, 2022
@Kebap Kebap deleted the patch-1 branch August 4, 2022 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants