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

gh-111999: Add signatures and improve docstrings for builtins #112000

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 12, 2023

@@ -110,9 +110,10 @@ bool_xor(PyObject *a, PyObject *b)
/* Doc string */

PyDoc_STRVAR(bool_doc,
"bool(x) -> bool\n\
"bool(object=False, /)\n\
Copy link
Member

Choose a reason for hiding this comment

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

Sphinx docs shows this as bool(x=False) (give impression that this is a positional-or-keyword argument). Probably, this should be adjusted too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe in other issue. The documentation not always uses syntax for positional-only parameters. It is relatively new.

\n\
(i.e. all elements that are in exactly one of the sets.)");
Return a new set with elements in either the set or other but not both.");
Copy link
Member

@skirpichev skirpichev Nov 12, 2023

Choose a reason for hiding this comment

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

BTW, for other set methods it seems there are signatures. But...

An example:

>>> help(set.add)
Help on method_descriptor:

add(self, object, /)
    Add an element to a set.

    This has no effect if the element is already present.

c.f. the sphinx docs:

add(elem)
    Add element elem to the set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Signatures for methods without arguments and with a single positional argument are now autogenerated.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, this looks like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

What exactly?

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping is obvious. Instead of mystic name object, I would expect something like this

>>> inspect.signature(set.add)
<Signature (self, element, /)>
>>> help(set.add)
Help on method_descriptor:

add(self, element, /)
    Add an element to a set.

    This has no effect if the element is already present.

I realize, parameter name here is not a part of API, but I think it should be meaningful, if possible.

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Nov 12, 2023

Choose a reason for hiding this comment

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

It could be improved, but it is not a bug. You can add explicit signatures for these methods, but for me it has low priority. I worked on support of multi-signatures, and this PR is a side product, the improvements that do not need multi-signatures (with multi-signatures and autogenerated signatures almost 100% of builtin functions and classes will have signatures).

Copy link
Member

Choose a reason for hiding this comment

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

I think 'object', an instance of the base object class, is as correct as 'element', which is a set-theory specific synonym. We might say that 'a list has items', but item is a synonym for object. Here also, I appreciate the technically correct list.append(self, object, /). I think the consistency of the auto-generated signatures is good.

Copy link
Member

Choose a reason for hiding this comment

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

One could argue that it should be set.add(self, hashable), as not any object can be added, whereas any object can be appended. The math term 'element' does not imply the restriction any more than 'object'.

@serhiy-storchaka serhiy-storchaka merged commit 1d75ef6 into python:main Nov 13, 2023
23 checks passed
@serhiy-storchaka serhiy-storchaka deleted the builtins-signatures branch November 13, 2023 07:13
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.

3 participants