Refactor the code generator

[pmaupin]

• Make it prettier
• Make it faster
• Make the interface to the pretty printer better

Currently, I really only care about to_source and rtrip (and tests for those). It's been years since I touched any of the other stuff.

So, I've been looking at refactoring all the kludgey pretty printing stuff I put in.

I think we should do this before the release, so that we don't release a kludgey pretty-printing API and then a better one.

I have code_gen refactored really well (I think) and the API over to the pretty printer is solid.

I would at least like to have it printing as well as it does now, preferably more PEP8 like. Right now, it doesn't do as many string changes as it used to, but it will be much easier to make the pretty printer better (and/or to replace it from external code), because all the string munging happens after the code generator is finished. (Before, the literal string fixups happened during the middle of code generation.)

Also, the code generator creates a list of lists, so the pretty printer doesn't have to separate the lines out -- they are already separated.

I'm tossing this out there to start the conversation. It needs PEP8 cosmetic changes as well, but I need to go to bed.

[pmaupin]

OK, @berkerpeksag I took your hint and removed the micro-optimizations.

The code looks a lot better, and now I think it performs a lot better, too. Stdlib roundtrips aren't nearly so ugly, because long strings are wrapped.

Anyway, this is all I have time for for now. I believe the code is in pretty good shape:

• It's slower than before the pretty printer, but much faster than a month ago
• All the stdlibs roundtrip fine
• Many more unittests than we had before
• There are no current bugs that I know of except non-ASCII source files under Python 2.0.

Still needs more doc and whatever new things we want to do to rtrip, but I don't know that we actually have anything gating a 0.6 release, given that the doc is better than it used to be.

[pmaupin]

In my local copy, I had made to_source a class method, and then backed it out, because I didn't think I had a use-case.

Then I remembered the use-case that was in the back of my mind -- issue #50 doesn't really need changes in astor itself, if the code is structured well enough, so I went ahead and checked in changes to support that.

Maybe later, after we have some experience and a lot of demand, we could make this a standard library feature, but I think the new test file showing how to add comment nodes should be more than good enough for anybody who wants to do that for now.

[radomirbosak]

What's the state of this Pull Request? Is anything blocking us from merging it?

@pmaupin mentions writing more docs - is this still needed?

As this PR sits open, it will become harder and harder to rebase it because we're making changes to astor.code_gen.py.

Since this PR was opened, we've merged 26 commits

$ git log --oneline 28b081a..master | wc -l
26

and changed these files:

$ git diff --stat 28b081a..master
 .travis.yml            |   5 +++
 AUTHORS                |   2 ++
 Makefile               |   6 ++--
 README.rst             |   6 ++--
 astor/__init__.py      |  11 ++----
 astor/code_gen.py      |  18 +++++-----
 astor/codegen.py       |  11 ++++++
 astor/rtrip.py         |  67 ++----------------------------------
 astor/tree_walk.py     |   2 +-
 docs/changelog.rst     | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 docs/index.rst         | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------
 setup.cfg              |  42 +++++++++++++++++++++++
 setup.py               |  56 ++++++++----------------------
 tests/support.py       |  44 ++++++++++++++++++++++++
 tests/test_code_gen.py |  18 +++++++---
 tests/test_misc.py     |  50 +++++++++++++++------------
 tox.ini                |   4 +--
 17 files changed, 453 insertions(+), 288 deletions(-)

However, the code_gen.py file changed minimally, so there shouldn't be any problem rebasing this PR.

$ git diff 28b081a..master -- astor/code_gen.py | xsel -b
diff --git a/astor/code_gen.py b/astor/code_gen.py
index 7c27f70..47d6acc 100644
--- a/astor/code_gen.py
+++ b/astor/code_gen.py
@@ -308,8 +308,8 @@ class SourceGenerator(ExplicitNodeVisitor):
         self.statement(node)
         self.generic_visit(node)

-    def visit_FunctionDef(self, node, async=False):
-        prefix = 'async ' if async else ''
+    def visit_FunctionDef(self, node, is_async=False):
+        prefix = 'async ' if is_async else ''
         self.decorators(node, 1 if self.indentation else 2)
         self.statement(node, '%sdef %s' % (prefix, node.name), '(')
         self.visit_arguments(node.args)
@@ -322,7 +322,7 @@ class SourceGenerator(ExplicitNodeVisitor):

     # introduced in Python 3.5
     def visit_AsyncFunctionDef(self, node):
-        self.visit_FunctionDef(node, async=True)
+        self.visit_FunctionDef(node, is_async=True)

     def visit_ClassDef(self, node):
         have_args = []
@@ -364,24 +364,24 @@ class SourceGenerator(ExplicitNodeVisitor):
                 self.else_body(else_)
                 break

-    def visit_For(self, node, async=False):
+    def visit_For(self, node, is_async=False):
         set_precedence(node, node.target)
-        prefix = 'async ' if async else ''
+        prefix = 'async ' if is_async else ''
         self.statement(node, '%sfor ' % prefix,
                        node.target, ' in ', node.iter, ':')
         self.body_or_else(node)

     # introduced in Python 3.5
     def visit_AsyncFor(self, node):
-        self.visit_For(node, async=True)
+        self.visit_For(node, is_async=True)

     def visit_While(self, node):
         set_precedence(node, node.test)
         self.statement(node, 'while ', node.test, ':')
         self.body_or_else(node)

-    def visit_With(self, node, async=False):
-        prefix = 'async ' if async else ''
+    def visit_With(self, node, is_async=False):
+        prefix = 'async ' if is_async else ''
         self.statement(node, '%swith ' % prefix)
         if hasattr(node, "context_expr"):  # Python < 3.3
             self.visit_withitem(node)
@@ -392,7 +392,7 @@ class SourceGenerator(ExplicitNodeVisitor):

     # new for Python 3.5
     def visit_AsyncWith(self, node):
-        self.visit_With(node, async=True)
+        self.visit_With(node, is_async=True)

     # new for Python 3.3
     def visit_withitem(self, node):

[pmaupin]

I think it was mergeable when made. Didn't want to do it because I thought a new release was imminent, and since it was a big change, wanted to do it after the big release.

I think it's a lot better than the old one; if it passes tests, should probably merge it.

On Fri, May 4, 2018 at 4:35 AM, Radomír Bosák notifications@github.com wrote:

What's the state of this Pull Request? Is anything blocking us from merging it?

@pmaupin https://github.com/pmaupin mentions writing more docs - is this still needed?

As this PR sits open, it will become harder and harder to rebase it because we're making changes to astor.code_gen.py.

Since this PR was opened, we've merged 26 commits

$ git log --oneline 28b081a..master | wc -l26

and changed these files:

$ git diff --stat 28b081a..master .travis.yml | 5 +++ AUTHORS | 2 ++ Makefile | 6 ++-- README.rst | 6 ++-- astor/init.py | 11 ++---- astor/code_gen.py | 18 +++++----- astor/codegen.py | 11 ++++++ astor/rtrip.py | 67 ++---------------------------------- astor/tree_walk.py | 2 +- docs/changelog.rst | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------- docs/index.rst | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------- setup.cfg | 42 +++++++++++++++++++++++ setup.py | 56 ++++++++---------------------- tests/support.py | 44 ++++++++++++++++++++++++ tests/test_code_gen.py | 18 +++++++--- tests/test_misc.py | 50 +++++++++++++++------------ tox.ini | 4 +-- 17 files changed, 453 insertions(+), 288 deletions(-)

However, the code_gen.py file changed minimally, so there shouldn't be any problem rebasing this PR.

$ git diff 28b081a..master -- astor/code_gen.py | xsel -bdiff --git a/astor/code_gen.py b/astor/code_gen.py index 7c27f70..47d6acc 100644--- a/astor/code_gen.py+++ b/astor/code_gen.py@@ -308,8 +308,8 @@ class SourceGenerator(ExplicitNodeVisitor): self.statement(node) self.generic_visit(node)

• def visit_FunctionDef(self, node, async=False):- prefix = 'async ' if async else ''+ def visit_FunctionDef(self, node, is_async=False):+ prefix = 'async ' if is_async else '' self.decorators(node, 1 if self.indentation else 2) self.statement(node, '%sdef %s' % (prefix, node.name), '(') self.visit_arguments(node.args)@@ -322,7 +322,7 @@ class SourceGenerator(ExplicitNodeVisitor):

  introduced in Python 3.5

  def visit_AsyncFunctionDef(self, node):- self.visit_FunctionDef(node, async=True)+ self.visit_FunctionDef(node, is_async=True)

  def visit_ClassDef(self, node): have_args = []@@ -364,24 +364,24 @@ class SourceGenerator(ExplicitNodeVisitor): self.elsebody(else) break

• def visit_For(self, node, async=False):+ def visit_For(self, node, is_async=False): set_precedence(node, node.target)- prefix = 'async ' if async else ''+ prefix = 'async ' if is_async else '' self.statement(node, '%sfor ' % prefix, node.target, ' in ', node.iter, ':') self.body_or_else(node)

  introduced in Python 3.5

  def visit_AsyncFor(self, node):- self.visit_For(node, async=True)+ self.visit_For(node, is_async=True)

  def visit_While(self, node): set_precedence(node, node.test) self.statement(node, 'while ', node.test, ':') self.body_or_else(node)

• def visit_With(self, node, async=False):- prefix = 'async ' if async else ''+ def visit_With(self, node, is_async=False):+ prefix = 'async ' if is_async else '' self.statement(node, '%swith ' % prefix) if hasattr(node, "context_expr"): # Python < 3.3 self.visit_withitem(node)@@ -392,7 +392,7 @@ class SourceGenerator(ExplicitNodeVisitor):

  new for Python 3.5

  def visit_AsyncWith(self, node):- self.visit_With(node, async=True)+ self.visit_With(node, is_async=True)

  new for Python 3.3

  def visit_withitem(self, node):

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/berkerpeksag/astor/pull/75#issuecomment-386550607, or mute the thread https://github.com/notifications/unsubscribe-auth/ACRNNmLDuRhU1AKEGHbvAAy1Q_sR-TXoks5tvCDGgaJpZM4NIZqT .

[berkerpeksag]

I think I'm the blocker here :) I was going to take a look Patrick's PRs (this one and #78) but unfortunately never had a chance to create some time for a review.