GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-115937: Remove implementation details from inspect.signature() docs by erlend-aasland · Pull Request #116086 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Feb 28, 2024 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Co-authored-by: Carol Willing [email protected]
Co-authored-by: Gregory P. Smith [email protected]
Co-authored-by: Jelle Zijlstra [email protected]

  • Issue: Incorrect description of the __signature__ attribute in docs #115937

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Gouvernathor reacted with thumbs down emoji
All reactions
  • 👎 1 reaction
@erlend-aasland @willingc
@gpshead @JelleZijlstra
pythongh-115937: Remove implementation details from inspect.signature…
72823f5
…() docs

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@erlend-aasland erlend-aasland added skip news needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 28, 2024
@erlend-aasland
erlend-aasland requested review from JelleZijlstra, gpshead and willingc February 28, 2024 23:40
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 28, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 28, 2024
Incorrect description of the __signature__ attribute in docs #115937
Closed
@bedevere-app bedevere-app Bot added the docs Documentation in the Doc dir label Feb 28, 2024
@erlend-aasland erlend-aasland linked an issue Feb 28, 2024 that may be closed by this pull request
Incorrect description of the __signature__ attribute in docs #115937
Closed
JelleZijlstra
JelleZijlstra reviewed Feb 28, 2024
View reviewed changes
Comment thread Doc/library/inspect.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland and others added 2 commits February 29, 2024 00:46
@erlend-aasland @JelleZijlstra
Update Doc/library/inspect.rst
15a5e30
Co-authored-by: Jelle Zijlstra <[email protected]>
@erlend-aasland
Reflow
8fb6a11
@Gouvernathor

Gouvernathor commented Feb 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland commented Feb 29, 2024
View reviewed changes
Comment thread Doc/library/inspect.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

gpshead
gpshead approved these changes Feb 29, 2024
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland and hauntsaninja reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 29, 2024
willingc
willingc approved these changes Feb 29, 2024
View reviewed changes

@willingc willingc left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@willingc

willingc commented Feb 29, 2024

Copy link
Copy Markdown
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).

erlend-aasland reacted with thumbs up emoji gpshead, erlend-aasland, and Gouvernathor reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Feb 29, 2024

Copy link
Copy Markdown
Member

Should I close #115984 in favor of this approach?

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Update Doc/library/inspect.rst
c1dec41
@erlend-aasland

erlend-aasland commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Should I close #115984 in favor of this approach?

Yes, please.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

erlend-aasland commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland mentioned this pull request Feb 29, 2024
gh-115937: remove extra processing for the __signature__ attribute #115984
Closed
@erlend-aasland
erlend-aasland merged commit fb2e17b into python:main Feb 29, 2024
@erlend-aasland
erlend-aasland deleted the docs/signature.inspect branch February 29, 2024 09:42
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 29, 2024
@miss-islington-app

This comment was marked as outdated.

Sign in to view
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 29, 2024
@erlend-aasland @willingc
@gpshead @JelleZijlstra @miss-islington
pythongh-115937: Remove implementation details from inspect.signature…
ac3fa69
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 29, 2024
@erlend-aasland @willingc
@gpshead @JelleZijlstra @miss-islington
pythongh-115937: Remove implementation details from inspect.signature…
4307a13
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@bedevere-app

bedevere-app Bot commented Feb 29, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Feb 29, 2024
@bedevere-app

bedevere-app Bot commented Feb 29, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@miss-islington @erlend-aasland
@willingc @gpshead @JelleZijlstra
[3.12] gh-115937: Remove implementation details from inspect.signatur…
31462d4
…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 <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
gpshead added a commit that referenced this pull request Feb 29, 2024
@miss-islington @erlend-aasland
@willingc @gpshead @JelleZijlstra
[3.11] gh-115937: Remove implementation details from inspect.signatur…
6e32837
…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 <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@Gouvernathor

Gouvernathor commented Feb 29, 2024

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

willingc reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Feb 29, 2024
Revert __signature__ to being a cache, remove string and callable support #116110
Closed
Re-document __signature__ as a feature #116124
Closed
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@erlend-aasland @willingc
@gpshead @JelleZijlstra @woodruffw
pythongh-115937: Remove implementation details from inspect.signature…
3fb4797
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@erlend-aasland @willingc
@gpshead @JelleZijlstra @adorilson
pythongh-115937: Remove implementation details from inspect.signature…
7efb06c
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@erlend-aasland @willingc
@gpshead @JelleZijlstra @diegorusso
pythongh-115937: Remove implementation details from inspect.signature…
250bb5b
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@gpshead gpshead gpshead approved these changes
@willingc willingc willingc approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect description of the __signature__ attribute in docs

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @Gouvernathor @willingc @skirpichev @gpshead @JelleZijlstra
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.