Skip to content

gh-109812: Fix phrasing for collections.Counter #109813

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 1 commit into from
Sep 28, 2023

Conversation

JacobCoffee
Copy link
Member

@JacobCoffee JacobCoffee commented Sep 25, 2023

This changes the wording for an incorrect collections.Counter piece.


📚 Documentation preview 📚: https://cpython-previews--109813.org.readthedocs.build/

@ghost
Copy link

ghost commented Sep 25, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! It is up to @rhettinger to decide on the wording, but the bug in the docs is clear! 👍

Congrats on your first CPython PR!
It is always nice to see familiar people 😉

@@ -358,7 +358,7 @@ Common patterns for working with :class:`Counter` objects::
list(c) # list unique elements
set(c) # convert to a set
dict(c) # convert to a regular dictionary
c.items() # convert to a list of (elem, cnt) pairs
c.items() # convert to a view object of the dictionary’s items (elem, cnt) pairs
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just

Suggested change
c.items() # convert to a view object of the dictionary’s items (elem, cnt) pairs
c.items() # convert to a iterator of (elem, cnt) pairs

?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not an iterator.

I'd just change "list" to "view". And "convert to" to "create", since the view doesn't really contain the data, so "convert to" seems wrong to me.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is just an iterable 👍

>>> {}.items().__iter__
<method-wrapper '__iter__' of dict_items object at 0x102cd1250>
>>> {}.items().__next__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'dict_items' object has no attribute '__next__'. Did you mean: '__ne__'?

@AA-Turner AA-Turner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 25, 2023
@@ -358,7 +358,7 @@ Common patterns for working with :class:`Counter` objects::
list(c) # list unique elements
set(c) # convert to a set
dict(c) # convert to a regular dictionary
c.items() # convert to a list of (elem, cnt) pairs
c.items() # create a view of the dictionary’s items (elem, cnt) pairs
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is more in line with the rest of the examples

Suggested change
c.items() # create a view of the dictionary’s items (elem, cnt) pairs
list(c.items()) # convert to a list of (elem, cnt) pairs

Copy link
Member

Choose a reason for hiding this comment

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

I disagree. The doc for c.items() should say what c.items returns.

@rhettinger
Copy link
Contributor

but the bug in the docs is clear!

Nitpick. The docs were correct when they were written (Python 2.7). It is the parent class that changed.

You all are welcome to change this to iterable which is correct but less accessible, or to view which is also correct and makes more specific promises, or to dict_items which is the exact type but meaningless to a reader, or to a set-like object providing a view on D's items which matches the inherited docstring. But ideally, you should focus on the communication goal for that section which is just a quick reference for common patterns for working with Counter objects. That section isn't a spec, it is a cheatsheet. So don't venture too far from what a newcomer would understand (i.e. 'view' and 'iterable' aren't accessible except for people are who already strong with Python).

This spirit of the section is about how we use counters, so I would opt for:

c.items() # access the (elem, cnt) pairs

@JacobCoffee
Copy link
Member Author

I agree on view being less accessible.. had to look up what that even was 🙃

- fail at squashing 7 commits for one line change

Signed-off-by: Jacob Coffee <[email protected]>
@JacobCoffee JacobCoffee reopened this Sep 26, 2023
@rhettinger rhettinger removed needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 28, 2023
@rhettinger rhettinger merged commit 99fba5f into python:main Sep 28, 2023
@JacobCoffee JacobCoffee deleted the gh-109812 branch September 28, 2023 02:49
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 29, 2023
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

6 participants