◐ Shell
reader mode source ↗
Skip to content

gh-108901: Add Signature.from_code method#108902

Closed
sobolevn wants to merge 7 commits into
python:mainfrom
sobolevn:issue-108901-getargs
Closed

gh-108901: Add Signature.from_code method#108902
sobolevn wants to merge 7 commits into
python:mainfrom
sobolevn:issue-108901-getargs

Conversation

@sobolevn

@sobolevn sobolevn commented Sep 5, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-bot bedevere-bot mentioned this pull request Sep 5, 2023
Open
@sobolevn

sobolevn commented Sep 5, 2023

Copy link
Copy Markdown
Member Author

Failures are not related: #108888

vstinner
vstinner reviewed Sep 5, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Please restrict this PR to add from_code(). Once it's merged, you can write a PR to deprecate.

@sobolevn sobolevn force-pushed the issue-108901-getargs branch from 95c2219 to 604f25b Compare September 5, 2023 16:57
@sobolevn sobolevn changed the title gh-108901: Deprecate inspect.getargs, use Signature.from_code instead Sep 5, 2023
vstinner
vstinner reviewed Sep 5, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I like your overall change, here is a first review.

@sobolevn

sobolevn commented Sep 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I moved my code, so there should be no unrelated diffs now :)

vstinner
vstinner approved these changes Sep 5, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM, but I would prefer a second core dev review, since I don't know well this module.

@vstinner

vstinner commented Sep 5, 2023

Copy link
Copy Markdown
Member

@pablogsal @AlexWaygood @1st1: Would you be interested to review this change?

AlexWaygood
AlexWaygood reviewed Sep 6, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

A few nits:

@sobolevn sobolevn requested a review from AlexWaygood September 8, 2023 11:10
@AlexWaygood

AlexWaygood commented Sep 8, 2023

Copy link
Copy Markdown
Member

Thanks for the PR, @sobolevn -- it was well done. But I think adding a constructor that implies you can construct a complete Signature from a code object is too misleading. Let's go with documenting that you should call inspect.signature(types.FunctionType(co, {})) in the rare cases that you don't have access to the original function object.

@AlexWaygood AlexWaygood closed this Sep 8, 2023
@sobolevn sobolevn mentioned this pull request Nov 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@vstinner vstinner vstinner approved these changes

@gpshead gpshead Awaiting requested review from gpshead

@1st1 1st1 Awaiting requested review from 1st1

@larryhastings larryhastings Awaiting requested review from larryhastings

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sobolevn @vstinner @AlexWaygood @merwok @JelleZijlstra @bedevere-bot