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
Add support for type annotations to attr:
ast parsing
#3391
Conversation
When walking the ast of a module, look for AnnAssign nodes in addition to Assign to support assignments with type annotations, for example. Since we have to read different attributes, split the generators into a for loop. Existing ast.Assign nodes follow the same processing as before.
…t does not have a value
Thank you very much for the contribution @karlotness (and the awesome attention to details, I completely forgot that the AST will handle typed assignments differently). I notice that the is one edge case that might break: attr: str The AST also parses this statement as an I will propose a change directly to your repository via PR (if you merge it, this PR is automatically updated). The suggestions I will send you will also include a way of having the generators back and avoid the duplication when raising the exception. |
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.
Please consider the fix suggested in karlotness#1 for the edge case when ast.AnnAssign
has a None
value
Refactor `StaticModule.__getattr__` and fix `ast.AnnAssign` edge case
That looks good, thanks. I hadn't considered the split type-annotation and later assignment. I took a look and you're right, looks like that would have led to an error and missed a later actual assignment. Thanks for taking a look and for the quick fix. |
Summary of changes
This extends the ast parsing setuptools does to load attributes without evaluating a module to also process simple assignments with type annotations. Those produce a different node
ast.AnnAssign
rather thanast.Assign
and require slightly different handling since the node has a different attributetarget
vstargets
.Since that seemed like it might make the generators more complicated, I reworked things into for loops and made some other tweaks. I'd be happy to take a different approach though.
The existing
ast.Assign
nodes should pass through exactly the same processing. TheAttributeError
that could result should also be the same too, except that it won't be chained with aStopIteration
if the outer for loop is exhausted (as it would be with thenext()
call).This doesn't close any existing open issue that I can find. There is #2563 which looks related. I think in that case the attribute loading worked because setuptools fell back to evaluating the module when the ast processing likely failed.
That fallback works in some cases, but it causes problems in when the module isn't importable until after the build (for example, extension modules that need to be compiled). The ast parsing is really helpful in these cases, and it would be nice if it could also handle type annotations.
Pull Request Checklist
changelog.d/
.(See documentation for details)