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

ekohilas · 2025-03-29T05:21:59Z

This PR updates the documentation of max and min to match their implementation like how sorted'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 with iterable 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 for max:

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 for min

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

Issue: Update function signatures to use * and / as needed #131885

ekohilas · 2025-03-29T05:39:10Z

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.

skirpichev

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 devguide suggests 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 actually simplify function signatures to avoid '/' and/or '*' syntax. See https://discuss.python.org/t/81003

CC @AA-Turner

Doc/library/functions.rst

skirpichev · 2025-03-29T06:00:07Z

Python/bltinmodule.c

@@ -2019,7 +2019,7 @@ builtin_min(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *k
 }

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


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

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:

cpython/Python/bltinmodule.c

Line 2575 in 5a3f13b

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

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

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

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.

Doc/library/functions.rst

ekohilas · 2025-03-30T01:24:07Z

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 devguide suggests 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 actually simplify function signatures to avoid '/' and/or '*' syntax. See https://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 like range 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.
- For max, * 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 that max 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.

skirpichev · 2025-03-30T01:29:30Z

Python/bltinmodule.c

@@ -2019,7 +2019,7 @@ builtin_min(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *k
 }

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


Suggested change

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

"min(iterable, /, *, key=None)\n\

min(iterable, /, *, default, key=None)\n\

min(arg1, arg2, *args, /, key=None)\n\

just like sphinx docs

Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>

ekohilas · 2025-03-30T01:37:17Z

Created issue: #131885

picnixz · 2025-03-30T09:28:25Z

While devguide suggests 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, the include is an imperative form and not a recommendation IMO. In addition, the rationale is (emphasis mine)

Although the syntax is terse, it is precise 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, albeit eval() could have used a positional-only at the end.

picnixz · 2025-03-30T09:32:56Z

Python/bltinmodule.c

@@ -2036,7 +2036,7 @@ builtin_max(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *k
 }

 PyDoc_STRVAR(max_doc,
-"max(iterable, *[, default=obj, key=func]) -> value\n\
+"max(iterable, /, *[, default=obj, key=func]) -> value\n\
 max(arg1, arg2, *args, *[, key=func]) -> value\n\


This needs a change as well.

picnixz · 2025-03-30T09:33:00Z

Python/bltinmodule.c

@@ -2019,7 +2019,7 @@ builtin_min(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *k
 }

 PyDoc_STRVAR(min_doc,
-"min(iterable, *[, default=obj, key=func]) -> value\n\
+"min(iterable, /, *[, default=obj, key=func]) -> value\n\
 min(arg1, arg2, *args, *[, key=func]) -> value\n\


This needs a change as well.

Suggestion was above;)

Ups, missed it :')

skirpichev · 2025-03-30T09:45:56Z

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. support bool(x) instead of bool(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 · 2025-03-30T10:00:41Z

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

picnixz · 2025-03-30T10:02:27Z

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 · 2025-03-30T10:03:22Z

Probably, yes. But rather than mechanically accept "complex" signatures - we should revisit on case-by-case basis if we could simplify them. E.g. support bool(x) instead of bool(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 · 2025-03-30T10:04:41Z

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

skirpichev · 2025-03-30T10:48:15Z

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 · 2025-03-30T15:23:31Z

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

AA-Turner

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 by pydoc, help(), etc. See Raymond's Discourse post on improving support for signatures, and Serhiy's draft work on multi-signature support for inspect.

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

A

bedevere-app · 2025-03-30T17:18:41Z

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.

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

skirpichev · 2025-03-31T01:49:12Z

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

Thanks! One thought; maybe we could mention that it's better to start with cases, when function signatures are simple, i.e. can be expressed with usual syntax for function definition in Python (e.g. round()).

@AA-Turner, honestly I don't sure I get why we should keep old signatures in docstrings.

First, we will have different signatures in the sphinx docs wrt docstrings - as an eternal source of bugs. Second, [,optional-arg]-syntax is not documented.

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 by pydoc, help(), etc.

Why not? I think it's a good idea to keep them in sync. BTW, I don't think that "C-style" signatures are interpreted somewhere.

See Raymond's Discourse post on improving support for signatures, and Serhiy's draft work on multi-signature support for inspect.

I doubt that current shortcomings of the inspect module really relevant here.

Yes, in the given example we can't accurately describe each function just by one signature with a correct Python syntax. But can with multiple, just as it's proposed in sphinx docs!

That's how it looks on top of the #117671 (I did some conflict resolution in skirpichev#7):

>>> help(max)
Help on built-in function max in module builtins:

max(iterable, /, *, key=None)
max(iterable, /, *, default, key=None)
max(arg1, arg2, /, *args, key=None)
    With a single iterable argument, return its biggest item. The
    default keyword-only argument specifies an object to return if
    the provided iterable is empty.
    With two or more positional arguments, return the largest argument.

>>> help(min)
Help on built-in function min in module builtins:

min(iterable, /, *, key=None)
min(iterable, /, *, default, key=None)
min(arg1, arg2, /, *args, key=None)
    With a single iterable argument, return its smallest item. The
    default keyword-only argument specifies an object to return if
    the provided iterable is empty.
    With two or more positional arguments, return the smallest argument.

>>> import inspect
>>> inspect.signatures(max)
<MultiSignature (iterable, /, *, key=None)|(iterable, /, *, default, key=None)|(arg1, arg2, /, *args, key=None)>

As you can see, function signatures exactly follow to proposal in this pr.

Updates functions.rst to make iterable positional only

7258061

bedevere-app bot added docs skip news labels Mar 29, 2025

ekohilas added 3 commits March 29, 2025 16:22

functions.rst for min

f5d4e7b

Update functions.rst

30eb1c0

Update bltinmodule.c for min and max

5a3f13b

ekohilas marked this pull request as ready for review March 29, 2025 05:26

ekohilas requested a review from ericsnowcurrently as a code owner March 29, 2025 05:26

bedevere-app bot added the awaiting review label Mar 29, 2025

skirpichev reviewed Mar 29, 2025

View reviewed changes

ekohilas commented Mar 30, 2025

View reviewed changes

Doc/library/functions.rst Outdated Show resolved Hide resolved

ekohilas commented Mar 30, 2025

View reviewed changes

Doc/library/functions.rst Outdated Show resolved Hide resolved

skirpichev reviewed Mar 30, 2025

View reviewed changes

Apply suggestions from code review

e8452dd

Co-authored-by: Evan Kohilas <ekohilas@users.noreply.github.com>

skirpichev changed the title ~~Updates docs to make max and min iterable param positional only~~ gh-131885: Updates docs to make max and min iterable param positional only Mar 30, 2025

bedevere-app bot mentioned this pull request Mar 30, 2025

Update function signatures to use * and / as needed #131885

Open

5 tasks

picnixz reviewed Mar 30, 2025

View reviewed changes

AA-Turner requested changes Mar 30, 2025

View reviewed changes

bedevere-app bot removed the awaiting review label Mar 30, 2025

bedevere-app bot added the awaiting changes label Mar 30, 2025

AA-Turner removed the request for review from ericsnowcurrently March 30, 2025 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ekohilas commented Mar 29, 2025 •

edited by bedevere-app bot

Loading

ekohilas commented Mar 29, 2025

skirpichev left a comment

skirpichev Mar 29, 2025

ekohilas Mar 30, 2025

ekohilas Mar 30, 2025

skirpichev Mar 30, 2025

picnixz Mar 30, 2025 •

edited

Loading

ekohilas commented Mar 30, 2025

skirpichev Mar 30, 2025 •

edited

Loading

ekohilas commented Mar 30, 2025

picnixz commented Mar 30, 2025

picnixz Mar 30, 2025

picnixz Mar 30, 2025

skirpichev Mar 30, 2025

picnixz Mar 30, 2025

skirpichev commented Mar 30, 2025

picnixz commented Mar 30, 2025

picnixz commented Mar 30, 2025

AA-Turner commented Mar 30, 2025

picnixz commented Mar 30, 2025

skirpichev commented Mar 30, 2025

picnixz commented Mar 30, 2025

AA-Turner left a comment

bedevere-app bot commented Mar 30, 2025

skirpichev commented Mar 31, 2025

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

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

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

Are you sure you want to change the base?

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

Conversation

ekohilas commented Mar 29, 2025 • edited by bedevere-app bot Loading

ekohilas commented Mar 29, 2025

skirpichev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Mar 30, 2025 • edited Loading

Choose a reason for hiding this comment

ekohilas commented Mar 30, 2025

skirpichev Mar 30, 2025 • edited Loading

Choose a reason for hiding this comment

ekohilas commented Mar 30, 2025

picnixz commented Mar 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirpichev commented Mar 30, 2025

picnixz commented Mar 30, 2025

picnixz commented Mar 30, 2025

AA-Turner commented Mar 30, 2025

picnixz commented Mar 30, 2025

skirpichev commented Mar 30, 2025

picnixz commented Mar 30, 2025

AA-Turner left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Mar 30, 2025

skirpichev commented Mar 31, 2025

ekohilas commented Mar 29, 2025 •

edited by bedevere-app bot

Loading

picnixz Mar 30, 2025 •

edited

Loading

skirpichev Mar 30, 2025 •

edited

Loading