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-115937: Remove implementation details from inspect.signature() docs #116086

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 28, 2024

Co-authored-by: Carol Willing carolcode@willingconsulting.com
Co-authored-by: Gregory P. Smith greg@krypto.org
Co-authored-by: Jelle Zijlstra jelle.zijlstra@gmail.com


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

…() docs

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
erlend-aasland and others added 2 commits February 29, 2024 00:46
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@Gouvernathor
Copy link
Contributor

Gouvernathor commented Feb 28, 2024

I disapprove of this way of solving the issue. @ethanfurman did not explain the rationale for changing the inspect module this way for the sole benefit of enum where it would have been just as easy not to (as #115984 shows).

The use of __signature__ as a cache - or as a lie to hide undocumented parameters - is too useful a feature to be undocumented.

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

I'd leave the "Consult the source code for current semantics." bit off as that feels implied to be when calling something "an implementation detail". But no big deal either way as it is true.

I do understand the Python user desire for officially describing and documenting __signature__ behavior as a public API people can rely on. That's outside the scope of this specific issue as this doc update is just trying to set expectations anywhere the existing implementation detail is mentioned.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

@erlend-aasland I think this documentation change with the implementation detail directive accurately reflects the current status of the underlying source code.

@willingc
Copy link
Contributor

I do understand the Python user desire for officially describing and documenting signature behavior as a public API people can rely on. That's outside the scope of this specific issue as this doc update is just trying to set expectations anywhere the existing implementation detail is mentioned.

@Gouvernathor As @gpshead mentions, I think having a separate (from documentation) discussion about behavior as a public API makes good sense. Also, I see you maintain ren'py, thanks for doing so. For years, my daughter used it to create interactive game scenarios (she is now a game developer).

@skirpichev
Copy link
Member

Should I close #115984 in favor of this approach?

@erlend-aasland
Copy link
Contributor Author

Should I close #115984 in favor of this approach?

Yes, please.

@erlend-aasland erlend-aasland enabled auto-merge (squash) February 29, 2024 09:38
@erlend-aasland
Copy link
Contributor Author

Thanks for the reviews!

@erlend-aasland erlend-aasland merged commit fb2e17b into python:main Feb 29, 2024
22 checks passed
@erlend-aasland erlend-aasland deleted the docs/signature.inspect branch February 29, 2024 09:42
@miss-islington-app

This comment was marked as outdated.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 29, 2024
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 29, 2024
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 29, 2024

GH-116106 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 29, 2024
@bedevere-app
Copy link

bedevere-app bot commented Feb 29, 2024

GH-116107 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 29, 2024
gpshead added a commit that referenced this pull request Feb 29, 2024
…e() docs (GH-116086) (#116106)

gh-115937: Remove implementation details from inspect.signature() docs (GH-116086)
(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
gpshead added a commit that referenced this pull request Feb 29, 2024
…e() docs (GH-116086) (#116107)

gh-115937: Remove implementation details from inspect.signature() docs (GH-116086)
(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@Gouvernathor
Copy link
Contributor

I think having a separate (from documentation) discussion about behavior as a public API makes good sense.

Following this, I think I will open a non-doc issue about reverting what #100039 changed about signatures.

But aside from that, and contrary to my first impression, that Enum PR didn't actually introduce an incompatible change. The __signature__ field accepted a Signature|None object since around 2014 (raising a TypeError for other types) and now accepts Signature|str|Callable[[], Signature]|None which is wider.
While I believe both of these additions are a bad design choice for reasons I explained in the linked issue, I think it would still be good to document that passing a Signature object makes it be returned by inspect.signature, that it's not an implementation detail (contrary to how this pull request presents it), and that any other value aside from None is undocumented and implementation-detail territory.
Such a formulation would keep the cachie/lie behavior I defended above and introduced in the original PEP to remain a documented feature.
I'm not sure how people intervening in this merge request would stand about these two propositions, so I would like feedback on that.


Also, I see you maintain ren'py, thanks for doing so.

You're very welcome. All the best to your daughter.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect description of the __signature__ attribute in docs
6 participants