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.
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-119180: PEP 649 compiler changes #119361
gh-119180: PEP 649 compiler changes #119361
Changes from all commits
985f8df
c822ffa
e80095e
90ff2c4
026c0ff
7968744
4e54197
4469b32
47d672e
ab9359c
e65d55a
e50cd62
afae5c0
e5a7b1a
3f26d44
31a4471
fbb1d88
f452eb2
8c4b4e3
cbf9a3d
5d182fc
ce98c19
e0578fc
ed16167
f38de20
87baca2
355d3df
f9d81b6
dd1f64a
62f5b3b
b66ad8b
1a63f5d
82c0dbc
a0c39b5
083bbc5
de1b235
1c98fe5
5f5cf11
77f3b1c
4217830
13f5d76
a121e1a
242301c
239ba23
b62e04c
c8a9294
5ae206d
24fd328
0f9d0c5
c181864
0befff5
ada6573
ae7714c
431811a
7ca24d3
2ab5d07
3b4a645
1dfd02b
daba318
0daf0b1
b066b3d
7669361
c6a1b80
5cdbdd7
e748feb
a2b4f9e
bd469ab
278de22
487ea34
8f486ba
6b563db
0058b82
517fb56
21f93b6
f72dbca
8674eab
ee11fd9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
(It would be great if we could generate a nice
__text_signature__
for these generated methods, but that definitely doesn't need to be done in this PR)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.
They're actually Python function, so I wonder why pydoc doesn't pick them up. Possibly because the parameter is called
.format
.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.
Indeed:
Maybe in a followup PR we can figure out a way to make this work. The parameter has to have an illegal name so it doesn't shadow any symbol name that the user might have used.
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'm not sure I 100% understand this point, and no tests seem to fail if I make this change locally to your PR branch and recompile (and I verified that it means that
__annotate__
methods have valid signatures as perinspect.signature
):This also might be worth a brief mention in PEP-749, since PEP-649 seems to specify that the signature of
__annotate__
should be__annotate__(format: int)
.In any event, this can definitely be tackled in a followup; I don't want to block this PR!
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.
Try something like this:
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 see, makes sense! Worth adding a test like that?
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.
Sure, the problem is that any such test can only tell us about one specific name.
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.
Right, it doesn't verify any fundamental invariants about
__annotate__
— but I still think it's useful to have such a test, so that the CI goes obviously red if someone else comes along in the future and tries to do the "obvious fix" to getinspect.signature()
working for these methods.Thanks for adding it!
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.
Why does printing
__annotations__
not work here? Shouldn't it call__annotate__
under the hood anyway?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.
Printing a global doesn't go through the descriptor protocol, so printing
__annotations__
will just throw a NameError; it is added to the namespace only when the__annotate__
descriptor is called.