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
bpo-34259: Improve docstring of list.sort #8516
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request You can check yourself Thanks again for your contribution, we look forward to reviewing it! |
Note: I've now signed the CLA. |
Hi @timhoffm and thank you for your contribution! Could you open an issue in the tracker and link it against this Pull Request as described in the devguide: https://devguide.python.org//pullrequest/? |
Thanks! We don't need a news item for this, but we will have to wait for the CLA signing to be approved. |
My overall suggestion, which is shorter and contains more information.
"""
Sort the list in ascending order and return None.
If reverse is True, sort in descending order instead. If a key function is given, apply it once to each list item and sort them, ascending or descending, according to their function values. In any case, the sort is stable -- items with equal values or equal key function values keep their order.
"""
Objects/listobject.c
Outdated
@@ -2145,12 +2145,21 @@ list.sort | |||
key as keyfunc: object = None | |||
reverse: bool(accept={int}) = False | |||
|
|||
Stable sort *IN PLACE*. | |||
Sort the list in ascending order. |
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.
Specifiy the return value. Add 'and return None'.
See next comment.
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.
Agree.
Objects/listobject.c
Outdated
|
||
A key function can be supplied to customize the sorting behavior. The key | ||
function is applied to each list element, and the elements are sorted | ||
according to the values returned by the key function. |
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.
More information in fewer words:
If a key function is given, apply it once to each list item and sort them, ascending or descending, according to their function values.
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.
Terry's version is better.
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 |
Objects/listobject.c
Outdated
@@ -2145,12 +2145,21 @@ list.sort | |||
key as keyfunc: object = None | |||
reverse: bool(accept={int}) = False | |||
|
|||
Stable sort *IN PLACE*. | |||
Sort the list in ascending order. |
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.
Agree.
Objects/listobject.c
Outdated
|
||
A key function can be supplied to customize the sorting behavior. The key | ||
function is applied to each list element, and the elements are sorted | ||
according to the values returned by the key function. |
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.
Terry's version is better.
@timhoffm, please address the code reviews and also please rebase to fix the merge conflict. Thanks! |
Sorry, this got lost. Will pick up again soon. |
@csabella Comments applied and rebased. |
Align the documentation for both `list.sort` and `sorted` since the latter's C implementation delegates to the C implementation of the former, in both docstrings, the library docs, and tutorial. Add a note about how reverse and sort stability interplay, as this is an easily forgotten edge case and may not necessarily always be desired. bltinmodule.c, listobject.c.h: * Instead of saying "equal elements" in the note about sort stability, we standardize on the "compare equal" phrasing from functions.rst, which is more precise phrasing that acknowledges that sorting is controlled by `key`. * "ascending or descending" is dropped from the explanation of `key` in `list.sort`, because that's controlled by `reverse`, not `key`. library/functions.rst: * Fix the type signature of `sorted`. * Redirect the documentation to `list.sort`. library/stdtypes.rst: * Fix the type signature of `list.sort`. * Replace the "as if each comparison were reversed" language with "descending order" for consistency. (Also, technically reversed isn't quite true: == comparisons aren't reversed ;).) tutorial/datastructures.rst: * Point the reference for `list.sort` to `list.sort`, instead of `sorted`. The set of modified files/locations was determined using: grep -C3 -rnE '`(list\.sort|sorted)`' . | grep -v 'howto/sorting\|whatsnew' The example of sort stability is included in functions.rst but omitted from the docstrings deliberately: you can only really get to it via `help()` or `pydoc`, which I suspect tend to be used by people who know what sort stability is. (Incidentally, in the discussion on pythonGH-8516, the last update to the list.sort docstring, rhettinger also expresses the POV that further discussion of sort stability belongs in extended docs like FAQs/HOWTOs.)
Improve docstring of list.sort
This PR extends the docstring of
list.sort
to explain the optional arguments.https://bugs.python.org/issue34259