Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-130881: Handle conditionally defined annotations#130935

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JelleZijlstra merged 11 commits intopython:mainfromJelleZijlstra:conditional-anno
Mar 26, 2025

Conversation

@JelleZijlstra
Copy link
Member

@JelleZijlstraJelleZijlstra commentedMar 7, 2025•
edited by bedevere-appbot
Loading

zzzeek reacted with thumbs up emoji
@JelleZijlstra
Copy link
MemberAuthor

Thanks@picnixz@tomasr8 for the reviews, please take another look!

@picnixzpicnixz self-requested a reviewMarch 12, 2025 14:42
Copy link
Member

@picnixzpicnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I won't force you to followPEP-7 as the file has a mix of everything which makes it very hard to choose a convention. You can also ignore my suggestion insymtable.c if you think it's not necessary and if the surrounding code is using the same writing style.

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

I think I reviewed the changes, though I'm not entirely sure about the tests themselves so another eye is welcome

@tomasr8
Copy link
Member

Once this is merged, should the text in PEP 649 be updated to reflect this change?

Code that sets annotations on module or class attributes from inside any kind of flow control statement. It’s currently possible to set module and class attributes with annotations inside an if or try statement, and it works as one would expect. It’s untenable to support this behavior when this PEP is active.

CaselIT and zzzeek reacted with thumbs up emoji

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Copy link
Member

@tomasr8tomasr8 left a comment

Choose a reason for hiding this comment

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

Just some comments/questions for the tests, otherwise looks good :)

@JelleZijlstra
Copy link
MemberAuthor

Once this is merged, should the text in PEP 649 be updated to reflect this change?

Code that sets annotations on module or class attributes from inside any kind of flow control statement. It’s currently possible to set module and class attributes with annotations inside an if or try statement, and it works as one would expect. It’s untenable to support this behavior when this PEP is active.

I don't own PEP 649 so I can't update it. Could add it to PEP 749 but I already submitted it to the SC.

Copy link
Member

@tomasr8tomasr8 left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me :)

@tomasr8
Copy link
Member

Once this is merged, should the text in PEP 649 be updated to reflect this change?

Code that sets annotations on module or class attributes from inside any kind of flow control statement. It’s currently possible to set module and class attributes with annotations inside an if or try statement, and it works as one would expect. It’s untenable to support this behavior when this PEP is active.

I don't own PEP 649 so I can't update it. Could add it to PEP 749 but I already submitted it to the SC.

Larry is the owner.@larryhastings should the PEP be updated?

@larryhastings
Copy link
Contributor

Accepted PEPs are historical documents; as a rule I'm averse to changing them after they're accepted. PEP 749 is the perfect place to document this change to what's specified in 649.

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

Two nits I missed but otherwise, looks good to me

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@JelleZijlstraJelleZijlstraenabled auto-merge (squash)March 26, 2025 03:21
@JelleZijlstraJelleZijlstra merged commit898e6b3 intopython:mainMar 26, 2025
42 checks passed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 1, 2025
seehwan pushed a commit to seehwan/cpython that referenced this pull requestApr 16, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@tomasr8tomasr8tomasr8 approved these changes

@picnixzpicnixzpicnixz approved these changes

@carljmcarljmAwaiting requested review from carljmcarljm is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JelleZijlstra@tomasr8@larryhastings@picnixz
[8]ページ先頭
©2009-2025 Movatter.jp