-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 682: Format Specifier for Signed Zero #2295
Conversation
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.
I think this looks good, with a few small corrections.
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.
Thanks! A couple of typos and clerical suggestions, and one bigger suggestion.
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.
Thanks, @belm0 , this looks great! I have a handful of comments from the RST technical side, as well as a few copyediting suggestions.
Before renaming the PEP to its number, to ensure they and those of the other reviewers don't get lost, I strongly recommend applying, declining or otherwise resolving them (using Github's Add to batch button and then the Commit button at the top on the Files
tab, making things easier for both you and reviewers) or not, as the case may be.
There are a few missing sections you probably want to include; they can be very brief (even just a line or two, in some cases) but its likely worth at least explicitly mentioning them:
- Backwards Compatibility: Presumably none, but it would be good to state this explicitly if so (and explain otherwise). You could at least briefly highlight how this proposal avoids the backward compatibility implications of changing the default behavior.
- Security Implications: If this is used in code, are there any foreseen implications for security? If so, you should describe them, and if not, you can simply state such explicitly. String formatting bugs (Log4Shell) have been all the rage lately, though this seems pretty safe.
- How to Teach This: It shouldn't need to be long, but it would be nice to at least briefly describe how you would suggest educating a newer user on when and how to use this feature.
- Rejected Ideas: If there have been any alternate ways of implementing this that have come up, or you think might, it would be worth at least briefly mentioning them here and why they were not pursued—e.g. change the default behavior? Name it something different from
z
? Make it a different part of the format specifier/format string mini-language?
Thanks again, and looking forward to seeing this as an official PEP!
Co-authored-by: CAM Gerlach <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Jelle Zijlstra <[email protected]>
fix example Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: CAM Gerlach <[email protected]>
* PEP assignment * missing email * correctness * remove speculation * wording * backwards compatibility
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.
One formatting comment (line-wrapping), three draughting comments.
A
pep-0682.rst
Outdated
The proposed extension is intentionally ``[sign][z]`` rather than | ||
``[sign[z]]``. The default for ``sign`` (``-``) is not widely known or | ||
explicitly written, so this avoids everyone having to learn it just to use | ||
the ``z`` option. |
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.
This is a matter of draughting, so you're well within your rights as author to ignore. It does feel a slight jump in topic though -- the previous paragraphs discuss the format specification in the round, and why the solution must be invisible to those not using it, etc. This paragraph though skips detail of the current format specification or grammar of the mini-langauge itself, going straight into the nested vs sibiling approach.
A possible solution would be to combine this paragraph with the preceeding, and say something along the lines of the solution must be opt in, and given that the negative option is the default we can't link the two, moving the detail of the grammar to the specification.
Another solution is to move this entire paragraph to the specification -- this could be argued to be explanation of the choices in the specification rather than rationale for the chage itself.
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.
Thank you. This is related to the discussion with CAM on section ordering: #2295 (comment). I originally wrote the rationale section intending that it support the decisions of the specification, and it was placed after the specification.
this could be argued to be explanation of the choices in the specification rather than rationale for the change itself.
Indeed, but as CAM cited from PEP 1:
"The rationale fleshes out the specification by describing why particular design decisions were made."
This suggests that the rational supports the specification, and is not merely a rationale for the change itself. If it does support the specification, logically it should follow that section. I wish that the guidelines on section ordering gave me such leeway.
Possible resolutions, with my preference first:
- move Rationale after Specification (perhaps naming it "Specification Rationale" for clarity)
- move some rationale text into the specification, as you suggest. (My view is that it makes the specification less concise, and I was happy to detail these things in another section.)
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.
Thanks for the extracts, I missed that discussion.
My own opinion is that clarity of expression is more important than adherence to the "rules". (That said, the "rules" exist for a reason!). I've also never been entirely sure of the difference between Motivation and Rationale.
I'd say flip the order if it makes the most sense.
@CAM-Gerlach -- would you have a strong objection to this?
A
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.
My actual suggestion was to not include this paragraph, the one following, and optionally the one preceding it in the Rationale at all:
Finally, you can make your last couple paragraphs separate subsections in the Rejected Ideas section (and maybe the opt-out too), since they each discuss why a particular alternative was not adopted, along with any you might add based on my suggestions above, and any others suggest.
They are rather detailed and implementation-level (as @AA-Turner noticed), and refer to why specific potential alternatives were not adopted, which is exactly what belongs in the Rejected Ideas section (with subheadings for each separate idea)—which, indeed, goes after the Specification, where you want them.
This just leaves the current mention of prior art (and any you add), the brief high-level rationale of the core design decision, and the counterpoint to why pre-rounding instead wouldn't work (the latter of which, if you really wanted, you could consider moving to Motivation or as a separate rejected idea), which makes much more sense as an introduction to the specification. If you prefer, you could also consider re-organizing those sub-parts a bit to put the pre-rounding approach first (why the status quo doesn't work), prior art section second (how did other languages tackle the problem) and the high-level description of your design last, which then smoothy outlines the rationale that led to the overall idea of the proposal, which then leads into the specification itself.
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.
Moving the Rationale after the Specification sounds good to me; I agree that clarity beats strict adherence to the rules.
I've also never been entirely sure of the difference between Motivation and Rationale.
Me neither, but there are two clearly distinct discussion items needed in a PEP like this: (1) the motivation for making a change (any change) in the first place, independent of the actual proposed solution, and (2) the reasons for choosing the particular solution that's being proposed from amongst other possible solutions. My best reading of PEP 12 is that these correspond to the Motivation and Rationale sections, respectively. And then if the Rationale section precedes the Specification section, it's a bit awkward to be referring to details of a specification that hasn't been presented yet.
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.
(2) the reasons for choosing the particular solution that's being proposed from amongst other possible solutions.
As I've been mentioning, the high level reasoning that led to the solution goes in the Rationale section, but the specific reasons why other possible solutions were not selected, and those dealing with the details of the specification, belong in the Rejected Ideas section, which goes after the Specification section. Is there a reason you'd rather not use that section for the more specific alternatives and details of the specification, while still keeping a short high-level rationale and comparison of prior art (which, certainly, should go before proposing what we're doing, its "prior" art after all) before? That seems like a good compromise that moves the stuff that you and Adam identified that doesn't really belong before the specification, after it, while still keeping the stuff that leads into it before and following PEP 1 and PEP 12 on the Rationale section itself?
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.
Is there a reason you'd rather not use that section for the more specific alternatives and details of the specification
No - I'll step out of this and leave the wrangling to you, Adam and John. I don't really care much about the exact order of the sections or whether there's strict adherence to PEP 12 or not - I care much more that the resulting text is coherent and is meaningful when read from top to bottom.
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.
I also don't have a strong opinion, having said my piece above!
I don't really care much about the exact order of the sections or whether there's strict adherence to PEP 12 or not - I care much more that the resulting text is coherent and is meaningful when read from top to bottom.
Seconded, although perhaps verboten for a pep editor to say :P
A
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 high level reasoning that led to the solution goes in the Rationale section, but the specific reasons why other possible solutions were not selected, and those dealing with the details of the specification, belong in the Rejected Ideas section
I would use the Rejected Ideas section if there was an idea that had legs and was actually rejected. To apply an opposite polarity to every design decision and render it as a rejected idea seems a little contrived. The items currently documented in Rationale, for the most part, had no reasonable alternatives-- and I touch on why that is for each. A (fictitious) example of something I'd put into Rejected Ideas would be "We explored automatically inferring suppression of negative zero based on whether the input was rounded, but it was found to be unreliable, and still may break existing programs."
I appreciate that the PEP guidelines are an established thing, and I'm merely a PEP author. On the other hand, my eyes are fresh to this, and there may be ways to refine the guidelines. At the least, I've come upon some contradiction in the guidelines as to whether Rationale is that of the entire PEP or of the Specification.
What I'd like to do from here is keep the current text, but as a "Specification Rationale" subsection within "Specification", so as to make the context clear. I hope it's an acceptable compromise, given that the sanctioned "Rationale" section is optional move the explanatory text to an Explanation subsection of Specification.
The attention from everyone towards making a good document has been impressive and welcome-- even if we have different views on it-- thank you.
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.
I think I've addressed what Adam raised originally by moving relevant text into Specification.
formatting Co-authored-by: Adam Turner <[email protected]>
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.
My recommendation for how to deal with the structure issue, and one small copyedit on the added text.
pep-0682.rst
Outdated
The proposed extension is intentionally ``[sign][z]`` rather than | ||
``[sign[z]]``. The default for ``sign`` (``-``) is not widely known or | ||
explicitly written, so this avoids everyone having to learn it just to use | ||
the ``z`` option. |
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.
My actual suggestion was to not include this paragraph, the one following, and optionally the one preceding it in the Rationale at all:
Finally, you can make your last couple paragraphs separate subsections in the Rejected Ideas section (and maybe the opt-out too), since they each discuss why a particular alternative was not adopted, along with any you might add based on my suggestions above, and any others suggest.
They are rather detailed and implementation-level (as @AA-Turner noticed), and refer to why specific potential alternatives were not adopted, which is exactly what belongs in the Rejected Ideas section (with subheadings for each separate idea)—which, indeed, goes after the Specification, where you want them.
This just leaves the current mention of prior art (and any you add), the brief high-level rationale of the core design decision, and the counterpoint to why pre-rounding instead wouldn't work (the latter of which, if you really wanted, you could consider moving to Motivation or as a separate rejected idea), which makes much more sense as an introduction to the specification. If you prefer, you could also consider re-organizing those sub-parts a bit to put the pre-rounding approach first (why the status quo doesn't work), prior art section second (how did other languages tackle the problem) and the high-level description of your design last, which then smoothy outlines the rationale that led to the overall idea of the proposal, which then leads into the specification itself.
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.
@belm0 Many thanks for writing this. It looks to me as though this is ready to go as soon as we resolve discussion on the ordering of the Motivation / Specification / Rationale pieces.
One question: do we want to add a "How to Teach This" section? I've taught many scores of Python classes, and feature creep in the language is a real concern for instructors trying to squeeze coverage of the core language into 1.5-2 days so that they can move onto Pandas / tensorflow / building GUIs / using Cython / whatever else. But from that perspective, this seems like a benign change: for those courses that cover the formatting specification language in detail, this would be an extra line with an example. For courses not going into detail it would make essentially no difference.
The other perspective is that of a developer encountering this in someone else's code and needing to understand what it's doing, but that also seems straightforward - they'd simply go to the appropriate section of the library manual, which would (presumably) contain the details of the use of z
. (It's probably too esoteric a feature to include those details the tutorial as well as the library reference.)
On other missing sections:
-
I don't think we need a "Security Implications" section - this is pretty clearly not security-adjacent, and all the section would say would be that there aren't any security implications that we're aware of.
-
I dare say we'll need a "Rejected Ideas" section as soon as discussion gets going, but that can wait for now.
Some other minor comments inline:
pep-0682.rst
Outdated
The proposed extension is intentionally ``[sign][z]`` rather than | ||
``[sign[z]]``. The default for ``sign`` (``-``) is not widely known or | ||
explicitly written, so this avoids everyone having to learn it just to use | ||
the ``z`` option. |
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.
Moving the Rationale after the Specification sounds good to me; I agree that clarity beats strict adherence to the rules.
I've also never been entirely sure of the difference between Motivation and Rationale.
Me neither, but there are two clearly distinct discussion items needed in a PEP like this: (1) the motivation for making a change (any change) in the first place, independent of the actual proposed solution, and (2) the reasons for choosing the particular solution that's being proposed from amongst other possible solutions. My best reading of PEP 12 is that these correspond to the Motivation and Rationale sections, respectively. And then if the Rationale section precedes the Specification section, it's a bit awkward to be referring to details of a specification that hasn't been presented yet.
Possible wording for "How to Teach This", based on how I'd approach teaching it in one of our Python courses: How to Teach ThisA typical introductory Python course will not cover string formatting in full detail. For such a course, no adjustments would need to be made. For a course that does go into details of the string formatting mini-language, a single example demonstrating the effect of the |
* fix "it's" ambiguity in rationale * add stack overflow links to Motivation (from Mark D.) * move specification support to Design Notes subsection * add How To Teach This (from Mark D.)
I'll merge this now as the basic text seems done & to allow posting to Discourse / python-dev. Improvements can be made in follow-up PRs if need be. A |
Thanks @belm0! A |
Thanks, all! I guess the next steps are to:
@belm0 Are you happy to do all of the above? I can open the discussion thread or send the python-dev post if that's helpful. |
.. code-block:: pycon | ||
|
||
>>> for x in (.002, -.001, .060): | ||
... print(f'{round(x, 1) + 0.: .1f}') | ||
0.0 | ||
0.0 | ||
0.1 |
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.
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.
Hmm, this isn't ideal. It's a pythondotorg styling problem, I'd imagine - and hard to fix currently.
A
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.
Grep shows only one other PEP using code-block:: pycon
, so there's no safety in numbers. I'll remove the usage for now, as part of #2317.
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.
Why would it be "pycon", and not "python"? It seems like all of the other PEPs don't specify a language, so I think you're right in removing it.
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.
Why would it be "pycon", and not "python"
pycon
is the Pygments lexer for python console text, versus python
which is for scripts. Semantically pycon
is correct here, although I don't know the exact difference between the two.
It seems like all of the other PEPs don't specify a language
Support for specifying languages was only added very recently (by adding Pygments to the requirements file) -- as a PEP editor I would strongly encourage explicitness in language as it makes reading PEPs easier with correctly highlighted code.
A
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 bit late now, sorry (I was invited to visit the SpaceX Starship production and launch site in Texas and thus was unavailable for the past week) but I can fully confirm everything @AA-Turner said, which was the basis of my initial implemented suggestion. In addition to the reasons he mentioned, being explicit about the language in the code block is also ensures the block will be highlighted correct regardless of what we decide to do in terms of the default syntax highlighter (none, auto, Python, etc). If pycon
is problematic, then the correct approach here is to just use python
instead, not remove this entirely.
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.
Thank you-- while we did revert the pycon block annotations for now, I agree that accurate block annotation is desirable and we'd like all the PEP documents to be doing that.
Summary from the time of the decision (#2317 (comment)):
- the console-specific formatting looks great with the new rendering system for peps.python.org
- however, rendering on the standard pep site is botched. @AA-Turner notes it may be hard to fix. (I'd be happy to help if it's feasible.)
- there is only one other PEP so far using
pycon
- it seems we'll need a script to convert console blocks in existing PEP's anyway, so perhaps no need to put this PEP on the bleeding edge
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.
Yes, but as I noted there, AFAIK all this is specific to the pycon
highlighter and not just simply using explicit .. code-block
markup which many PEPs are now using; simply using the python
highlighter instead should fix the issue (unless you've confirmed it doesn't, but I didn't find any mention of that here nor there). And I am not aware of plans for batch-converting all old PEPs to use the new explicit language markup, except in cases on active/process PEPs where the highlighting is widely off (wrong language, etc) but there's no reason not to be explicit and correct in new ones as @AA-Turner says, unless it is not compatible with both renderers.
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.
batch-converting all old PEPs to use the new explicit language markup
I have avoided this topic as I don't want to spark a discussion. My current opinion is we should leave implicit literal block syntax as it is in most places, but update all cases where the language is not Python. This opinion is subject to change several times per arbitrary time period.
This probably means we wouldn't update to pycon
here on a retroactive sweep.
typefaces
another debate for another time ;)
A
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.
I have avoided this topic as I don't want to spark a discussion. My current opinion is we should leave implicit literal block syntax as it is in most places, but update all cases where the language is not Python. This opinion is subject to change several times per arbitrary time period.
I share this opinion as well, and am willing to help implement if and when the time comes for this, though I think it could be limited to not include withdrawn, superceded, and otherwise inactive PEPs. For new PEPs I would favor the .. code-block:: python
syntax for all code blocks, which I believe is your viewpoint as well.
No description provided.