bpo-47122: Fix the table of methods in the collections.abc documentation #32090

geryogam · 2022-03-23T22:59:42Z

This pull request makes the following changes to the table of methods in the collections.abc documentation:

Reversible: add __iter__ abstract method;
Generator: replace __iter__ with inherited mixin Iterator method;
MutableSequence: add clear mixin method;
Set: remove __ne__ mixin method (not defined here but in object), add __rand__ mixin method, add __ror__ mixin method, add __rsub__ mixin method, add __rxor__ mixin method;
Mapping: remove __ne__ mixin method (not defined here but in object);
ItemsView: add inherited mixin MappingView method and inherited mixin Set methods;
KeysView: add inherited mixin MappingView method and inherited mixin Set methods;
ValuesView: add inherited mixin MappingView method;
Coroutine: add __await__ abstract method;
AsyncGenerator: replace __aiter__ with inherited mixin AsyncIterator method;
footnotes: remove footnote 2 which is a duplicate of collections.abc.Iterable description.

https://bugs.python.org/issue47122

Doc/library/collections.abc.rst

rhettinger · 2022-03-25T20:30:14Z

Doc/library/collections.abc.rst

-:class:`Collection`  [1]_      :class:`Sized`,        ``__contains__``,
+:class:`Reversible` [1]_       :class:`Iterable`      ``__reversed__``,
+                                                      ``__iter__``
+:class:`Generator` [1]_        :class:`Iterator`      ``send``,               Inherited mixin :class:`Iterator` method,


I would leave the Generator as is. The "inherited" references are used only when the would copy a lot of methods. The current form for this entry is more readable, useful, and easy to understand.

Doc/library/collections.abc.rst

rhettinger · 2022-03-25T20:43:44Z

Doc/library/collections.abc.rst

                                                      ``__setitem__``,        ``pop``, ``popitem``, ``clear``, ``update``,
-                                                      ``__delitem__``,        and ``setdefault``
+                                                      ``__delitem__``,        ``setdefault``
                                                      ``__iter__``,


Please don't do these minor stylistic edits. It makes the table harder to read.

I reverted most of them.

Doc/library/collections.abc.rst

rhettinger · 2022-03-25T20:51:49Z

Doc/library/collections.abc.rst

                                                      ``__iter__``,           ``clear``, ``pop``, ``remove``, ``__ior__``,
-                                                      ``__len__``,            ``__iand__``, ``__ixor__``, and ``__isub__``
+                                                      ``__contains__``,       ``__iand__``, ``__ixor__``, ``__isub__``


This Github diff makes it difficult to see what exactly what is changing here. Are you taking out the required len method, if so why?

Also put the "and" back in.

Sorry for the unfriendly diff. I did not remove __len__, I just reordered the abstract methods from contains, iter, len to len, iter, contains, so that it matches the order of the inherited classes Sized, Iterable, Container of Collection to improve readibility.

I also removed the ‘and’ for readability as it looks like the __and__ method, and because it was not used consistently in the table (every list of three items or more should have it).

What do you think?

rhettinger · 2022-03-25T20:54:05Z

Doc/library/collections.abc.rst

-                               :class:`Set`                                   ``__iter__``
-:class:`ValuesView`            :class:`MappingView`,                          ``__contains__``, ``__iter__``
-                               :class:`Collection`
+:class:`ItemsView`             :class:`MappingView`,                          Inherited mixin :class:`MappingView` method,


This group of edits is harsh on the eyes. I don't see how they add any value. ISTM these all refactor the presentation style in a way that makes the table even harder to read and use.

rhettinger · 2022-03-25T20:58:47Z

Doc/library/collections.abc.rst

@@ -220,7 +213,7 @@ Collections Abstract Base Classes -- Detailed Descriptions
   ABC for classes that provide the :meth:`__iter__` method.

   Checking ``isinstance(obj, Iterable)`` detects classes that are registered
-   as :class:`Iterable` or that have an :meth:`__iter__` method, but it does
+   as or inherit from :class:`Iterable`, or that have an :meth:`__iter__` method, but it does


This is probably unnecessary. First, isinstance() always detects direct inheritance so that does need to be repeated every time we talk about isinstance(). Second, all classes that inherit from Iterable will already have an iter method so that case is covered by the current sentence.

bedevere-bot · 2022-03-25T21:02:24Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

rhettinger · 2022-03-25T21:08:06Z

Can you make a simplified version of this PR that sticks to fixing entries that are incorrect (like the missing clear method) rather than stylistic edits (eliding "and", general restyling, propagating the "inherited from" wording, general wording edits, etc.)?

This table has been very successful in help users, but it is already challenging to read. IMO a number of these edits make less approachable or useful for day to day programming. With some of the edits a user would be better of running dir(cls) or cls.__abstractmethods__ than trying to read the revised table.

geryogam · 2022-03-29T19:22:42Z

Thanks for the review. Sorry for the unfriendly diff: I have reverted the reordering of the methods and the removal of newlines after multiline entries. I have also restored footnote 2 and removed the propagation of the ‘inherited from’ wording for single inherited methods as suggested. Finally, I have commented about the elided ‘and’. This will hopefully make the P.R. more reviewable.

I have made the requested changes; please review again.

bedevere-bot · 2022-03-29T19:22:45Z

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

merwok · 2022-06-14T14:31:34Z

Doc/library/collections.abc.rst


-:class:`MutableSequence`       :class:`Sequence`      ``__getitem__``,        Inherited :class:`Sequence` methods and
+:class:`MutableSequence`       :class:`Sequence`      ``__getitem__``,        Inherited mixin :class:`Sequence` methods,


What is the benefit of adding the word mixin here?

A downside is that readers may wonder if mixin methods or inherited mixin methods are a special thing, which they aren’t. So I would not add the word, inherited Sequence methods is fine.

It is to refer only to mixing methods of the parent class, as the other parent methods do not belong to this column.

Add inheritance to footnotes

cad1102

bedevere-bot added the awaiting review label Mar 23, 2022

the-knights-who-say-ni added the CLA signed label Mar 23, 2022

bedevere-bot added the docs Documentation in the Doc dir label Mar 23, 2022

geryogam changed the title ~~Add inheritance to footnotes~~ Improve collections.abc documentation Mar 23, 2022

geryogam added 2 commits March 24, 2022 09:03

Remove a repetition

1497a14

Fix methods in the table of ABCs

0e8ee06

geryogam changed the title ~~Improve collections.abc documentation~~ Fix the table of methods in the collections.abc documentation Mar 25, 2022

geryogam marked this pull request as ready for review March 25, 2022 18:22

geryogam requested a review from rhettinger as a code owner March 25, 2022 18:22

geryogam changed the title ~~Fix the table of methods in the collections.abc documentation~~ bpo-47122: Fix the table of methods in the collections.abc documentation Mar 25, 2022

Fix typo

3467071

rhettinger self-assigned this Mar 25, 2022

rhettinger added the skip news label Mar 25, 2022

rhettinger reviewed Mar 25, 2022

View reviewed changes

Doc/library/collections.abc.rst Show resolved Hide resolved

rhettinger reviewed Mar 25, 2022

View reviewed changes

rhettinger requested changes Mar 25, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Mar 25, 2022

bedevere-bot added the awaiting changes label Mar 25, 2022

geryogam added 4 commits March 29, 2022 19:52

Revert the "inherited" references for single methods, revert footnote 2

0c61afb

Remove trailing whitespace

8cb8da8

Revert method order changes

d0b9012

Restore newlines after multiline entries

bb2318a

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 29, 2022

bedevere-bot requested a review from rhettinger March 29, 2022 19:22

merwok reviewed Jun 14, 2022

View reviewed changes

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into patch-30

18b319d

bpo-47122: Fix the table of methods in the collections.abc documentation #32090

bpo-47122: Fix the table of methods in the collections.abc documentation #32090

geryogam commented Mar 23, 2022 •

edited

rhettinger Mar 25, 2022 •

edited

rhettinger Mar 25, 2022

geryogam Mar 29, 2022

rhettinger Mar 25, 2022

geryogam Mar 29, 2022

rhettinger Mar 25, 2022

rhettinger Mar 25, 2022

bedevere-bot commented Mar 25, 2022

rhettinger commented Mar 25, 2022

geryogam commented Mar 29, 2022

bedevere-bot commented Mar 29, 2022

merwok Jun 14, 2022

geryogam Oct 4, 2022 •

edited


		:class:`MutableSequence` :class:`Sequence` ``__getitem__``, Inherited :class:`Sequence` methods and
		:class:`MutableSequence` :class:`Sequence` ``__getitem__``, Inherited mixin :class:`Sequence` methods,

bpo-47122: Fix the table of methods in the collections.abc documentation #32090

Are you sure you want to change the base?

bpo-47122: Fix the table of methods in the collections.abc documentation #32090

Conversation

geryogam commented Mar 23, 2022 • edited

rhettinger Mar 25, 2022 • edited

Choose a reason for hiding this comment

rhettinger Mar 25, 2022

Choose a reason for hiding this comment

geryogam Mar 29, 2022

Choose a reason for hiding this comment

rhettinger Mar 25, 2022

Choose a reason for hiding this comment

geryogam Mar 29, 2022

Choose a reason for hiding this comment

rhettinger Mar 25, 2022

Choose a reason for hiding this comment

rhettinger Mar 25, 2022

Choose a reason for hiding this comment

bedevere-bot commented Mar 25, 2022

rhettinger commented Mar 25, 2022

geryogam commented Mar 29, 2022

bedevere-bot commented Mar 29, 2022

merwok Jun 14, 2022

Choose a reason for hiding this comment

geryogam Oct 4, 2022 • edited

Choose a reason for hiding this comment

geryogam commented Mar 23, 2022 •

edited

rhettinger Mar 25, 2022 •

edited

geryogam Oct 4, 2022 •

edited