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

Add rudimentary support for python files that use type hints #56

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Add rudimentary support for python files that use type hints #56

merged 1 commit into from
Jan 12, 2021

Conversation

DragonMinded
Copy link
Contributor

Pretty much as it says on the tin. I was adding type hints to a small python library that used pscript (https://github.com/DragonMinded/firebeatrtc) and when I went to regenerate the js I ran into lack of support for AnnAssign. This is incredibly basic support, since I don't bother to do anything with AnnAssign nodes that don't actually assign a value (like x: int) aside from raising an exception. I also threw in a hack to allow importing from "typing" as a whitelisted import with similar rationale to the existing whitelisted entries.

Do I have to edit other parsers with similar whitespace or run any tests? I didn't run anything aside from seeing that after this change, pscript successfully generated the js file exactly as I expected it to.

@almarklein
Copy link
Member

Thanks for this!

I had to look this up, so adding it for the record: the AnnAssign AST node is an assignment + annotation. E.g.

foo = 3   # results in assign node
foo: int = 3  #  results in AnnAssign node

So this change will work with such code, by simply dropping the annotation.

This looks good to me, but it would be great if a test could be added in this file to make sure that this new feature continues to work. See e.g. this line to run the test only on specific Python versions.

@DragonMinded
Copy link
Contributor Author

Technically the grammar for AnnAssign allows for annotation without assignment. So, the following:

foo = 3  # results in Assign node
foo: int = 3  # results in AnnAssign node
foo: int  # valid python, results in a type annotation for foo and a blank value in the AnnAssign _ast node

For the third case, I didn't know what to do (should we define the variable, should we propagate type information, should we just ignore the node?) so I throw an exception. Its a much bigger conversation than this simple PR was attempting to address. But yes, your analysis is spot on, it supports these types of assignments by just dropping the annotation. I checked the output before and after adding type hints for the code I cared about and the JS was identical so I was happy.

I can definitely add a test or two.


def _convert_AnnAssign(self, n):
if n.value is None:
raise RuntimeError("Cannot convert AnnAssign nodes with no assignment!")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to RuntimeError because that's what all other errors in this file were.

@almarklein
Copy link
Member

Its a much bigger conversation than this simple PR was attempting to address

Indeed. It would be very interesting to use type information in pscript (see #4), but it would be a huge effort :)

Thanks again for this!

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