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

fix: typing.Annotated should be special-cased like typing.Literal #193

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

Daverball
Copy link
Collaborator

@Daverball Daverball commented Nov 13, 2024

This is a partial fix for #192

This is probably not a huge change, since we skipped unexpected nodes within annotations already, so it would mostly get rid of some false positives where a string within Annotated happened to match one of the imported symbols and thus being treated as a forward reference incorrectly.

I still need to think about a solution for string annotations and whether or not we should visit the nodes we're currently skipping, maybe we should pass self into AnnotationVisitor so we can call the generic_visit of the outer visitor?

@Daverball
Copy link
Collaborator Author

I ended up switching to actually parsing string annotations with ast.parse rather than trying to keep getting away with a regex.

It's still not perfect, since starred expressions are only allowed within a subscript and there may come a time where further context specific syntax restrictions may complicate this even further. But for now I think this should be good enough.

Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

Looks good to me @Daverball!

@Daverball Daverball merged commit 28c5381 into snok:main Nov 13, 2024
6 checks passed
@Daverball Daverball deleted the fix/typing-annotated branch December 12, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants