Skip to content

bpo-36144: Document PEP 584. #18659

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

Merged
merged 6 commits into from
Feb 26, 2020
Merged

bpo-36144: Document PEP 584. #18659

merged 6 commits into from
Feb 26, 2020

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Feb 25, 2020

@@ -4392,6 +4392,22 @@ pairs within braces, for example: ``{'jack': 4098, 'sjoerd': 4127}`` or ``{4098:
>>> d.values() == d.values()
False

.. method:: d | other
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use ‘describe’ instead of‘method’. (What do the docs for ‘set’ use?)

Copy link
Member Author

Choose a reason for hiding this comment

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

set uses method, but it lists both the method and operator together. I agree that describe is probably better here.

Merge (``|``) and update (``|=``) operators have been added to the built-in
:class:`dict` class.

.. XXX The rest is just a copy-paste of the PEP's Specification section.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe drop the rest and link to the PEP instead? The comment above praises brevity. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the brevity bit is about the summary section, no? I latched onto "Do not spend very much time on the wording of your changes"... hence, the copy-paste. But you're right that it probably does not add much here.

Merge (``|``) and update (``|=``) operators have been added to the built-in
:class:`dict` class.

.. XXX https://www.python.org/dev/peps/pep-0584/
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you would want to keep a full link to the PEP in a comment. Do other sections do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I misunderstood your prior review comment!

Comment on lines 76 to 83
Merge (``|``) and update (``|=``) operators have been added to the built-in
:class:`dict` class.

.. XXX https://www.python.org/dev/peps/pep-0584/

See :pep:`584` for a full description.

(Contributed by Brandt Bucher in :issue:`36144`.)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, looking at the rendered docs, and comparing other sections, I wonder if this shouldn't just be a single paragraph.

@brandtbucher
Copy link
Member Author

@gvanrossum I'm a bit confused. Looking at the "New Features" section of the What's New for 3.8, I see somewhat detailed docs with examples spread across several paragraphs (for example, for assignment expressions and positional-only parameters). The smaller, non-PEP items below are more compact though.

However, I'm probably overthinking this, which is exactly what the comments at the top warn against. 🙂

I'll just keep the short version and the notes in one paragraph, as you suggest, and Łukasz or whoever will go back later and expand as needed, then?

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

It's true, we could add more. But the feature is so simple, I think it doesn't need more. (It's not new syntax.)

@gvanrossum
Copy link
Member

I'll land it now. If you feel regrets later (or someone else thinks the whatsnew section on dict|dict should have more information), just submit a PR with a doc update, referencing the same issue (https://bugs.python.org/issue36144).

@gvanrossum gvanrossum merged commit d0ca9bd into python:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants