Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-131885: Updates docs to make max and min iterable param positional only#131868

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
AA-Turner merged 6 commits intopython:mainfromekohilas:patch-1
Aug 11, 2025

Conversation

@ekohilas
Copy link
Contributor

@ekohilasekohilas commentedMar 29, 2025
edited by bedevere-appbot
Loading

This PR updates the documentation ofmax andmin to match their implementation like howsorted's implementation matches it's documentation.

Sorted's function definition in the documentation is as follows:

sorted(iterable, /, *, key=None, reverse=False)

When attempted to call withiterable as a keyword argument, the following occurs:

>>> sorted(iterable=[1])Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: sorted expected 1 argument, got 0

However, looking at the documentation formax:

max(iterable, *, key=None)

And comparing to their implementation:

>>> max(iterable=[1])Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: max expected at least 1 argument, got 0

The same follows formin


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

@bedevere-appbedevere-appbot added docsDocumentation in the Doc dir skip news labelsMar 29, 2025
@ekohilasekohilas marked this pull request as ready for reviewMarch 29, 2025 05:26
@ekohilas
Copy link
ContributorAuthor

Actually, I'm finding more examples of these cases such as map and range. If the above looks good I'll add more commits to include those too.

Copy link
Contributor

@skirpichevskirpichev left a comment

Choose a reason for hiding this comment

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

Actually, I'm finding more examples of these cases such as map and range. If the above looks good I'll add more commits to include those too.

Previously similar change was reverted:#99476 See also#125945

While devguidesuggests now using correct function signatures, I'm not sure we shold push this that way.

We definitely need an issue, e.g. to adjust all signatures on the builtin functions page. We should also consider if we could actuallysimplify function signatures toavoid'/' and/or'*' syntax. Seehttps://discuss.python.org/t/81003

CC@AA-Turner


PyDoc_STRVAR(min_doc,
"min(iterable, *[, default=obj, key=func]) -> value\n\
"min(iterable,/,*[, default=obj, key=func]) -> value\n\
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a valid Python syntax. Same for max().

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I understand the issue being that the square brackets make it invalid syntax, and that without this change, the documentation would still be wrong.

The reason for change here is to ensure that the web docs align with the code docs, similar to sorted:

"sorted($module, iterable, /, *, key=None, reverse=False)\n"

Would it be more correct to instead write:

Suggested change
"min(iterable, /, *[, default=obj,key=func]) -> value\n\
"min(iterable, /, *,key=None) -> value\n\
min(iterable, /,*,default,key=None)->value\n\
min(arg1,arg2,*args, /,*,key=None)->value\n\

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@skirpichev Could you please help me understand why-> value should be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rationale is just same as for sphinx docs: it's not a valid syntax (you will get a NameError trying to parse such signature withinspect.signature()).

Copy link
Member

@picnixzpicnixzMar 30, 2025
edited
Loading

Choose a reason for hiding this comment

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

I understand the issue being that the square brackets make it invalid syntax, and that without this change, the documentation would still be wrong.

Actually, it's a valid Sphinx syntax. It's the "legacy" way of writing signatures. Many built-ins are written that way. As for->, I think it was meant for "readability" even if it's unparsable. I would personally leave this untouched in this PR (namely, only add positional-only marker but leave out the->). In a follow-up, we could clean-up the way "invalid" syntaxes, but this is something different than adding the markers.

ekohilas reacted with thumbs up emoji
@ekohilas
Copy link
ContributorAuthor

Actually, I'm finding more examples of these cases such as map and range. If the above looks good I'll add more commits to include those too.

Previously similar change was reverted:#99476 See also#125945

Thanks for these references!

While devguidesuggests now using correct function signatures, I'm not sure we shold push this that way.

We definitely need an issue, e.g. to adjust all signatures on the builtin functions page. We should also consider if we could actuallysimplify function signatures toavoid'/' and/or'*' syntax. Seehttps://discuss.python.org/t/81003

Thank you for linking this read!

I too would like to see simplifying these built-in function signatures, and have functions likerange allow for keyword arguments.

As I understand it, doing so would be a one day door that would require additional thought and decisions, as if it went through and started to be used, it would no longer be backwards compatible.

Until then, this seems like the next best step. It's what the Editorial board has agreed on, it brings consistency, and reverting these changes in the future if and when they are simplified would be easy.

As it stands, without these changes:

  • The documentation is inconsistent and causes confusion. e.g.
    • Formax,* is used to highlight how the function works, but/ isn't, even though it behaves like it is.
    • There is no reference in the docs to specify thatmax doesn't allow some arguments as keyword.
  • It's difficult for users to understand what the correct usage is, or how to re-create these functions themselves. If a user copies this function definition to do so, it would not match the real definition.

@ekohilas
Copy link
ContributorAuthor

Created issue:#131885

@skirpichevskirpichev changed the titleUpdates docs to make max and min iterable param positional onlygh-131885: Updates docs to make max and min iterable param positional onlyMar 30, 2025
@picnixz
Copy link
Member

While devguidesuggests now using correct function signatures, I'm not sure we shold push this that way.

Personally, I read this is as requirement and not a suggestion. Namely,

If a function accepts positional-only or keyword-only arguments, include the slash and the star in the signature as appropriate:

Here, theinclude is an imperative form and not a recommendation IMO. In addition, the rationale is (emphasis mine)

Although the syntax is terse, it isprecise about the allowable ways to call the function and is taken from Python itself.

So I think it's fine to accept those changes. The reversal of previous changes was mostly done because it was incorrect, albeiteval() could have used a positional-only at the end.

@skirpichev
Copy link
Contributor

So I think it's fine to accept those changes.

Probably, yes. But rather than mechanically accept "complex" signatures - we should revisit on case-by-case basis if we could simplify them. E.g. supportbool(x) instead ofbool(x, /).

The reversal of previous changes was mostly done because it was incorrect

@picnixz, are you about#99476? Reversion was argued not by correctness:#98340

@picnixz
Copy link
Member

Wait, were you talking about#100547 or another reversal?

@picnixz
Copy link
Member

Ah no I see. Ok you were talking about another PR. My bad. Well, the EB decision is quite recent and the reversal was done before that decision so I think it doesn't apply here.

@AA-Turner
Copy link
Member

Probably, yes. But rather than mechanically accept "complex" signatures - we should revisit on case-by-case basis if we could simplify them. E.g. supportbool(x) instead ofbool(x, /).

For this reason, I'm not sure the tracking issue is helpful -- it will encourage people to create many PRs to 'fix' the issue, when it is not just a mechanical transformation at hand.

@picnixz
Copy link
Member

I'm not sure the tracking issue is helpful

This is the reason why I didn't add the "easy" label. But tracking is fine I think. I can add a warning note to say that it's not an easy one as the transformation is not just mechanical

AA-Turner reacted with thumbs up emoji

@skirpichev
Copy link
Contributor

I'm not sure the tracking issue is helpful -- it will encourage people to create many PRs to 'fix' the issue, when it is not just a mechanical transformation at hand.

Someone could comment on issue to clarify it further.

picnixz reacted with thumbs up emoji

@picnixz
Copy link
Member

I've added a red caution box, hopefully it will be seen

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

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

We should avoid mixing C-style[,optional-arg] with the/ etc markers.

I think we should also not conflate changes to the documentation, which support a wider range of syntax, with changes to function signatures in docstrings, which are interpreted bypydoc,help(), etc. See Raymond's Discourse post on improving support for signatures, and Serhiy's draft work on multi-signature support forinspect.

For now, please revert the changes tobltinmodule.c, and we can start with iterative improvements to the rST documentation.

A

ekohilas reacted with thumbs up emoji
@bedevere-app
Copy link

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

And if you don't make the requested changes, you will be put in the comfy chair!

@skirpichev
Copy link
Contributor

@ekohilas, please avoid force-pushing to the PR.

@ekohilas
Copy link
ContributorAuthor

ekohilas commentedApr 12, 2025
edited
Loading

@ekohilas, please avoid force-pushing to the PR.

Ah sorry I'm clicking the rebase button via GitHub.

Are there guidelines on why I should avoid force pushing in the cpython repo?

@skirpichev
Copy link
Contributor

Sure, they listed in the devguide:https://devguide.python.org/getting-started/pull-request-lifecycle/#id2

ekohilas reacted with heart emoji

Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

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

In#137610, Serhiy proposes to put/s everywhere they belong, and I strongly support this. HIs PR#137609 includes the 2 additions here, so he also approves them. I think that this should be merged first so that OP gets the proper credit.

@terryjreedy
Copy link
Member

@AA-Turner You requested removal of changes from the .c module, and this was done. Anything else? or dismiss change?

@AA-TurnerAA-Turner added needs backport to 3.13bugs and security fixes needs backport to 3.14bugs and security fixes labelsAug 11, 2025
@AA-TurnerAA-Turner merged commitdd079db intopython:mainAug 11, 2025
31 of 32 checks passed
@miss-islington-app
Copy link

Thanks@ekohilas for the PR, and@AA-Turner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@github-project-automationgithub-project-automationbot moved this fromTodo toDone inDocs PRsAug 11, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestAug 11, 2025
…)`` (pythonGH-131868)(cherry picked from commitdd079db)Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestAug 11, 2025
…)`` (pythonGH-131868)(cherry picked from commitdd079db)Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>
@bedevere-app
Copy link

GH-137656 is a backport of this pull request to the3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14bugs and security fixes labelAug 11, 2025
@bedevere-app
Copy link

GH-137657 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelAug 11, 2025
AA-Turner pushed a commit that referenced this pull requestAug 11, 2025
…()`` (GH-131868) (#137657)Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>
maurycy added a commit to maurycy/cpython that referenced this pull requestAug 12, 2025
* main:pythongh-137288: Update 3.14 magic numbers (pythonGH-137665)pythongh-135228: When@DataClass(slots=True) replaces a dataclass, make the original class collectible (take 2) (pythonGH-137047)pythongh-126008: Improve docstrings for Tkinter cget and configure methods (pythonGH-133303)pythongh-131885: Use positional-only markers for ``max()`` and ``min()`` (python#131868)pythonGH-137426: Remove code deprecation of `importlib.abc.ResourceLoader` (pythonGH-137567)pythongh-125897: Mark range function parameters as positional only (python#125945)pythongh-137400: Fix a crash when disabling profiling across all threads (pythongh-137471)pythongh-115766: Fix IPv4Interface.is_unspecified (pythonGH-137326)pythongh-128813: cleanup C-API docs for PyComplexObject (pythonGH-137579)pythongh-135953: Profile a module or script with sampling profiler (python#136777)  Fix documentation of hash in PyHash_FuncDef (python#137595)
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull requestAug 19, 2025
AA-Turner pushed a commit that referenced this pull requestOct 7, 2025
…()`` (GH-131868) (#137656)gh-131885: Use positional-only markers for ``max()`` and ``min()`` (GH-131868)(cherry picked from commitdd079db)Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@skirpichevskirpichevskirpichev left review comments

@picnixzpicnixzpicnixz left review comments

@AA-TurnerAA-TurnerAA-Turner approved these changes

@terryjreedyterryjreedyterryjreedy approved these changes

+1 more reviewer

@XuehaiPanXuehaiPanXuehaiPan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docsDocumentation in the Doc dirskip news

Projects

Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ekohilas@picnixz@skirpichev@AA-Turner@terryjreedy@XuehaiPan

[8]ページ先頭

©2009-2025 Movatter.jp