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

Allow specifying the generator class. #114

Merged
merged 5 commits into from
May 19, 2019

Conversation

matham
Copy link
Contributor

@matham matham commented Jun 28, 2018

Fixes #113
Adds the ability to specify the class to be used to generate the source code.

@matham
Copy link
Contributor Author

matham commented Jun 29, 2018

I have another related question. I would like to be able to adjust the maxline parameter passed to split_lines in to_source. Would it make sense if I added maxline to the function signature and forwarded it to split_line?

Copy link
Owner

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This looks good to me, it would be great if we could add some tests and documentation:

  1. For tests, adding just two tests would be enough: 1) we can convert the example I gave at Allow registering new visit_xxx for SourceGenerator #113 (comment) 2) and we also need a test to cover the TypeError branch -- see my review comments about this

  2. For documentation, we need to update the documentation in docs/index.rst. Please add a ..versionchanged:: 0.7 Added *source_generator_class* parameter. marker (feel free to change the wording, I just gave it as an example.)

  3. Please update docs/changelog.rst and add your name to AUTHORS file.

astor/code_gen.py Show resolved Hide resolved
@berkerpeksag
Copy link
Owner

Would it make sense if I added maxline to the function signature and forwarded it to split_line?

Could you open a new issue to discuss this one? There is an open PR to refactor astor/source_repr.py and its friends, so maybe we can come up with a better solution.

@matham
Copy link
Contributor Author

matham commented Aug 7, 2018

I just wanted to update that I have not forgotten about the PR, it's just that I ended up using astor quite a bit in new code I wrote, so before I continued with the PR I wanted to get a clearer picture for myself how I was going to use astor to make sure my solution was good approach.

In the end, I just wrote my own version of to_source (https://github.com/matham/kivy/blob/ast_compile/kivy/lang/compiler/ast_parse.py#L320) because I needed further customization, so I'm wondering if I should continue with this PR, or if it's better that the user make their own to_source, like I did rather than making to_source more complex? Especially if the API will be changing as you mentioned?

If the underlying API used in to_source is available more clearly, maybe that's better than adding more stuff to to_source? But either way, I can add tests etc to this PR or drop it. Whichever you think is better.

@berkerpeksag
Copy link
Owner

I think this PR would still help users to customize source generator. I recently came across a blog post using astor (I can't read Chinese, but I can read Python :)): http://blog.soliloquize.org/2017/02/08/%E4%BD%BF%E7%94%A8ast%E7%A7%BB%E9%99%A4%E4%BB%A3%E7%A0%81%E4%B8%AD%E7%9A%84print%E8%AF%AD%E5%8F%A5/

With source_generator_class, the latest example can be written without needing to subclass ast.NodeTransformer:

import astor

class RemovePrintGenerator(astor.code_gen.SourceGenerator):

    def visit_Call(self, node):
        if node.func.id != 'print':
            super().visit_Call(node)

print(astor.to_source(astor.parse_file('foo.py'), source_generator_class=RemovePrintGenerator))

@berkerpeksag berkerpeksag merged commit cdedca5 into berkerpeksag:master May 19, 2019
@matham matham deleted the patch-1 branch May 19, 2019 17:30
@matham
Copy link
Contributor Author

matham commented May 19, 2019

Thanks for finishing this off. I hadn't had the time to complete it.

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.

Allow registering new visit_xxx for SourceGenerator
2 participants