Skip to content

Conversation

@cdonovick
Copy link
Collaborator

This is a bit janky I automatically rebuild python ast class tree in an immutable form.

It might be better to run _generate_immutable_ast in setup.py instead of shipping a prebuilt immutable_ast.py.

@cdonovick cdonovick requested a review from leonardt August 30, 2019 19:34
@cdonovick
Copy link
Collaborator Author

Coverage increased on last push I don't know why coveralls thinks it went down...

@cdonovick
Copy link
Collaborator Author

cdonovick commented Aug 30, 2019

Another option would be to just use the generated code as a starting point which we edit from here out. I think this has a couple of advantages.

It would simplify the metaclass as it would not longer need to dynamically construct __setattr__, __delattr__, __hash__, __eq__, __ne__ as they could just be moved to immutable_ast.AST.

It would make the code more clean because the re-implementations of iter_child_nodes, NodeVisitor, etc... could check isinstance(node, AST) instead of isinstance(type(node), ImmutableMeta) as immutable_ast.AST would actually be visible.
see https://github.com/leonardt/ast_tools/blob/immutable/ast_tools/_immutable_ast.py#L32

The disadvantage would obviously be that it is tied to a specific python version.

A final option would be to specialize the generation of immutable.AST and template in everything including the metaclass and functions to achieve the outcome of the above without the manual effort. However, this would probably make maintenance somewhat more difficult as everything would be in strings instead of code.

@cdonovick
Copy link
Collaborator Author

The more I think about the more I think that dynamically generating immutable_ast.py is a good idea while certain passes may be tied to a specific version of the ast keeping ast_tools as agnostic to the version as possible seems like a good idea.

@cdonovick cdonovick force-pushed the immutable branch 2 times, most recently from 6ecac85 to bb965e8 Compare September 3, 2019 00:10
@cdonovick
Copy link
Collaborator Author

cdonovick commented Sep 3, 2019

Okay I have generating ast_tools.immutable_ast in setup.py kinda. However, I don't think my current approach will work on pypi because I do not think util/ will exist in pypi and I don't know how to make it be there without installing it or putting everything in setup.py.

While I think it would be pretty cool to automatically build immutable_ast on the fly I think it will ultimately be a nightmare for maintenance. So I think it might be best to actually just track the file even though it is generated. If people want to use a version of the AST not in our pypi then they can just generate it themselves.

@cdonovick
Copy link
Collaborator Author

Another option is generate a bunch of versions and import the one we want.

package structure:

ast_tools/immutable_ast/__init__.py
ast_tools/immutable_ast/ast35.py
ast_tools/immutable_ast/ast36.py
ast_tools/immutable_ast/ast37.py
ast_tools/immutable_ast/ast38.py
...

ast_tools/immutable_ast/__init__.py:

if sys.version_info[:2] == (3, 5):
  from ast35 import *
elif sys.version_info[:2] == (3, 6)
  from ast36 import *
...


def __eq__(self, other):
if not isinstance(other, type(self)):
return NotImplemented
Copy link
Owner

Choose a reason for hiding this comment

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

Could this just be False?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess this will try the other way (inverse) which may be okay too, this seems reasonable.

# s/list/tuple/
#
# The CPython license is very permissive so I am pretty sure this is cool.
# If it is not Guido please forgive me.
Copy link
Owner

Choose a reason for hiding this comment

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

😂

Copy link
Owner

@leonardt leonardt left a comment

Choose a reason for hiding this comment

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

LGTM

@leonardt leonardt merged commit aa4ef49 into master Sep 3, 2019
@leonardt leonardt deleted the immutable branch September 3, 2019 17: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.

3 participants