Skip to content

gh-100989: Revert #100990: Improve the accuracy of collections.deque docstrings #102949

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

Conversation

timobrembeck
Copy link
Contributor

@erlend-aasland
Copy link
Contributor

I verify that this PR fixes the IDLE tooltip regression of #100990.

Screenshot 2023-03-23 at 15 01 54

@timobrembeck timobrembeck force-pushed the gh-100989-fix-regression-in-collections-deque-docstring branch from e4ae5a1 to 317f39b Compare March 23, 2023 14:07
@erlend-aasland
Copy link
Contributor

Please do not force-push; instead do git merge --no-ff main if you want to pull in recent changes.

The devguide also mentions this.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM; let's wait for Raymond to chime in.

@rhettinger
Copy link
Contributor

The original commits should be reverted entirely. There is no known user problem that warrants a wholesale rewrite of the docstrings. In addition, the existing docstrings were modeled after their counterparts in list and tuple. I don't want to lose that relationship:

for method in set(dir(list)) & set(dir(deque)):
    if not method.startswith('__'):
        print(method.upper())
        print('list:  ', getattr(list, method).__doc__)
        print('deque: ', getattr(list, method).__doc__)
        print()

        
EXTEND
list:   Extend list by appending elements from the iterable.
deque:  Extend list by appending elements from the iterable.

CLEAR
list:   Remove all items from list.
deque:  Remove all items from list.

COPY
list:   Return a shallow copy of the list.
deque:  Return a shallow copy of the list.

APPEND
list:   Append object to the end of the list.
deque:  Append object to the end of the list.

REMOVE
list:   Remove first occurrence of value.

Raises ValueError if the value is not present.
deque:  Remove first occurrence of value.

Raises ValueError if the value is not present.

INDEX
list:   Return first index of value.

Raises ValueError if the value is not present.
deque:  Return first index of value.

Raises ValueError if the value is not present.

REVERSE
list:   Reverse *IN PLACE*.
deque:  Reverse *IN PLACE*.

COUNT
list:   Return number of occurrences of value.
deque:  Return number of occurrences of value.

POP
list:   Remove and return item at index (default last).

Raises IndexError if list is empty or index is out of range.
deque:  Remove and return item at index (default last).

Raises IndexError if list is empty or index is out of range.

INSERT
list:   Insert object before index.
deque:  Insert object before index.

I've taught Python to new people weekly for 11 years. I've seen no evidence at all that these docstrings cause any issues for beginners.

@timobrembeck
Copy link
Contributor Author

@rhettinger Alright, I appreciate the insight!

The motivation for this was not that I think the current docstrings are unclear in any way, but simply that I think my changes provide a compromise that improves compatibility with existing conventions and tools while not degrading understandability for humans at the same time.
If the second part is not true, I agree that readability for humans is more important.

I mean of course I could also adapt the docstrings of their counterparts if this is the main concern, but as I understand your comments, this is more of a matter of principle, and probably other problems will arise if we go down that road.

@rhettinger
Copy link
Contributor

rhettinger commented Mar 23, 2023

In the absence of a known user problem with readability, we should leave the docstrings in sync with listobject.c. Also, docstrings edits that aren't bug fixes are not supposed to be backported. In the initial review, I asked that only one change be made, going from "integer" to "int" and noted that all the other edits were gratuitous. Please stick with just that. Side ntoe, it is the duty of third-party tools to match CPython conventions, not vice-versa. These docstrings have been present for well over a decade. All third party tools should have been tested against them.

@rhettinger
Copy link
Contributor

@erlend-aasland Are you on-board with closing this PR and reverting the others? The original PR did not follow my review comments. It violated our docstring pep resulting which affected tooling and degraded the user experience. It broke the intentional symmetry with listobject.c. It did not address any known user readability issue. The "fix accuracy" checkin message bordered on being intentionally misleading. And since it wasn't a bug fix, it should not have been backported, especially without the consent of the module maintainer.

If there are new docstring conventions that we want to require, they should be proposed as an amendment to the docstring PEP rather than being pushed through under the guise of "fixing accuracy".

@timobrembeck
Copy link
Contributor Author

In the initial review, I asked that only one change be made, going from "integer" to "int" and noted that all the other edits were gratuitous.

Ok, I reverted all changes but this one. When this PR is squash merged, it will revert the old PR and include only the change from integer to int.

@timobrembeck
Copy link
Contributor Author

If there are new docstring conventions that we want to require, they should be proposed as an amendment to the docstring PEP rather than being pushed through under the guise of "fixing accuracy".

The existing docs mention PEP-7 which says:

The first line of each function docstring should be a “signature line” that gives a brief synopsis of the arguments and return value. For example:

PyDoc_STRVAR(myfunction__doc__,
"myfunction(name, value) -> bool\n\n\
Determine whether name and value make a valid pair.");

Always include a blank line between the signature line and the text of the description.

If the return value for the function is always None (because there is no meaningful return value), do not include the indication of the return type.

So a first step would be to update the docs and state that PEP-7 is no longer the preferred format.

@timobrembeck
Copy link
Contributor Author

Also, PEP-257, which was mentioned in the discussion, says:

The one-line docstring should NOT be a “signature” reiterating the function/method parameters (which can be obtained by introspection). Don’t do:

def function(a, b):
    """function(a, b) -> list"""

This type of docstring is only appropriate for C functions (such as built-ins), where introspection is not possible.

And especially the last paragraph sounds to me as this format would in fact be appropriate for C functions.

@erlend-aasland
Copy link
Contributor

@rhettinger: sure, I'm fine with reverting #100990, and AFAICS, this PR has now been updated to do just that (except for the integer => int thing).

@erlend-aasland erlend-aasland changed the title gh-100989: Fix regression in docstrings of collections.deque gh-100989: Revert #100990: Improve the accuracy of collections.deque docstrings Mar 23, 2023
@rhettinger
Copy link
Contributor

Thanks. In the interest of making sure it is done correctly and not overlooking any spurious changes, I'm going to do clean 100% revert of the previous PRs. A new clean PR can be made for just the "integer" to "int" change and applied only to 3.12 and 3.11.

@rhettinger rhettinger closed this Mar 23, 2023
@erlend-aasland
Copy link
Contributor

SGTM

@timobrembeck timobrembeck deleted the gh-100989-fix-regression-in-collections-deque-docstring branch March 24, 2023 16:57
@hauntsaninja hauntsaninja removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants