flexxui / pscript

Python to JavaScript compiler
http://pscript.readthedocs.io
BSD 2-Clause "Simplified" License
260 stars 25 forks source link

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

Closed DragonMinded closed 3 years ago

DragonMinded commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

almarklein commented 3 years ago

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!