Skip to content
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

Merged
merged 1 commit into from Jun 1, 2019

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented Jul 28, 2018

Improve docstring of list.sort

This PR extends the docstring of list.sort to explain the optional arguments.

https://bugs.python.org/issue34259

@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Jul 28, 2018

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
and a Python core developer will remove the CLA not signed label
to make the bot check again.

You can check yourself
to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@timhoffm
Copy link
Contributor Author

timhoffm commented Jul 28, 2018

Note: I've now signed the CLA.

@pablogsal
Copy link
Member

pablogsal commented Jul 28, 2018

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/?

@timhoffm timhoffm changed the title Improve docstring of list.sort bpo-34259: Improve docstring of list.sort Jul 28, 2018
@zooba
Copy link
Member

zooba commented Jul 28, 2018

Thanks! We don't need a news item for this, but we will have to wait for the CLA signing to be approved.

Copy link
Member

@terryjreedy terryjreedy left a comment

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.
"""

@@ -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.
Copy link
Member

@terryjreedy terryjreedy Aug 3, 2018

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.

Copy link
Contributor

@rhettinger rhettinger Aug 11, 2018

Choose a reason for hiding this comment

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

Agree.

Objects/listobject.c Show resolved Hide resolved
Objects/listobject.c Show resolved Hide resolved
Objects/listobject.c Show resolved Hide resolved

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.
Copy link
Member

@terryjreedy terryjreedy Aug 3, 2018

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.

Copy link
Contributor

@rhettinger rhettinger Aug 11, 2018

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.

@bedevere-bot
Copy link

bedevere-bot commented Aug 3, 2018

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.

@@ -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.
Copy link
Contributor

@rhettinger rhettinger Aug 11, 2018

Choose a reason for hiding this comment

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

Agree.

Objects/listobject.c Show resolved Hide resolved
Objects/listobject.c Show resolved Hide resolved

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.
Copy link
Contributor

@rhettinger rhettinger Aug 11, 2018

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.

Objects/listobject.c Show resolved Hide resolved
@csabella
Copy link
Contributor

csabella commented May 19, 2019

@timhoffm, please address the code reviews and also please rebase to fix the merge conflict. Thanks!

@timhoffm
Copy link
Contributor Author

timhoffm commented May 19, 2019

Sorry, this got lost. Will pick up again soon.

@timhoffm
Copy link
Contributor Author

timhoffm commented May 31, 2019

@csabella Comments applied and rebased.

@csabella csabella requested review from rhettinger and terryjreedy Jun 1, 2019
@rhettinger rhettinger merged commit 5c22476 into python:master Jun 1, 2019
5 checks passed
@timhoffm timhoffm deleted the list-sort-docstring branch Jun 2, 2019
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
sxlijin pushed a commit to sxlijin/cpython that referenced this pull request May 9, 2020
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.)
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.

None yet

9 participants