Handle new ast.Constant node in SourceGenerator #121

chrisrink10 · 2018-09-30T14:30:29Z

This PR adds SourceGenerator.visit_Constant to handle Python 3.8 deprecating older constant AST node types. I reused a lot of the logic from the exist node types by breaking them out into private handlers for the node value. For a few of the node types this ended up being a little gnarly, so let me know if you have some other ideas for how to organize this code. I didn't want to duplicate a lot of the functionality, since there was a lot there (especially for strings and numeric types) that was probably learned with a lot of bug reports.

I'm not sure exactly how easily you can test this specific code until the Python compiler is emitting these constant nodes directly, but I'm happy to look into it more if you'd like. Locally (at least) the existing tests were passing for me.

Fixes #120

berkerpeksag · 2018-09-30T15:18:54Z

Thanks for the PR! It looks pretty good to me. I think your organization of the code seems reasonable. I will post my nitpicks later today :)

Perhaps we could try to generate something like spam = 'eggs' from an AST tree generated by Python 3.8?

chrisrink10 · 2018-10-01T12:30:51Z

@berkerpeksag I added a few test cases (which are currently failing) to illustrate how I was thinking about testing this change. I'll try to look into why they are failing a little later.

chrisrink10 · 2018-10-02T00:47:18Z

Alright @berkerpeksag I fixed the tests now. The only thing we might want to do is add Python 3.8-dev into the Travis test suite, but I'll leave that up to you.

berkerpeksag · 2018-10-02T06:30:56Z

Thank you very much! I think adding 3.8-dev into Travis is a good idea.

chrisrink10 · 2018-10-02T12:24:17Z

Alright! Python 3.8 dev builds working.

serhiy-storchaka · 2018-10-03T16:41:30Z

astor/code_gen.py

+    # ast.Ellipsis, ast.NameConstant, ast.Num, ast.Str in Python 3.8
+    def visit_Constant(self, node):
+        value = node.value
+        if isinstance(value, (complex, float, int)):


bool will be handled here.

serhiy-storchaka · 2018-10-03T16:57:40Z

For most values the code is just self.write(repr(value)). You need to handle just few special values: float and complex numbers with inf and nan and Ellipsis. For some reasons strings need special handling too.

chrisrink10 · 2018-10-04T11:58:40Z

Thanks for the comments @serhiy-storchaka ! I just want to clarify - do you want me to remove the private handlers and just use self.write(repr(value)) except for numbers and strings?

serhiy-storchaka · 2018-10-04T12:28:41Z

I would write the Constant visitor as

    def visit_Constant(self, node):
        value = node.value
        if isinstance(value, (float, complex)):
            self._handle_numeric_constant(value)
        elif isinstance(value, str):
            precedence = self.get__pp(node)
            self._handle_string_constant(value, precedence)
        else:
            self.write(repr(value))

and make the NameConstant handler using repr() instead of str() for uniformity.

berkerpeksag · 2018-10-04T13:17:12Z

Serhiy's suggestion sounds good to me.

chrisrink10 · 2018-10-04T22:41:51Z

Thanks for the comments @berkerpeksag and @serhiy-storchaka . I've implemented the changes you suggested.

serhiy-storchaka

I think changes in _handle_string_constant() can be minimized. Other than that all LGTM as the first step.

I see some bugs in existing code, but that is for other issues.

serhiy-storchaka · 2018-10-05T06:02:29Z

astor/code_gen.py


+    def _handle_string_constant(self, value, precedence, join_fn=None):


Why not pass the original node? It could be used for precedence and for recursion. You will don't need to repeat precedence = self.get__pp(node).

serhiy-storchaka · 2018-10-05T06:14:06Z

tests/test_code_gen.py

+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Name(id='None')),
+            "spam = None")
+
+    @unittest.skipUnless((3, 4) <= sys.version_info <= (3, 8),


I think <= (3, 8) can be removed.

serhiy-storchaka · 2018-10-05T06:15:58Z

tests/test_code_gen.py

+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.NameConstant(value=None)),
+            "spam = None")
+
+    @unittest.skipIf(sys.version_info >= (3, 8),


I think the skip should be removed. These tests work in 3.8 and will continue to work in several future versions.

chrisrink10 · 2018-10-05T11:53:27Z

Thank you for reviewing again @serhiy-storchaka ! I pushed some changes which addressed your latest round of comments.

serhiy-storchaka · 2018-10-05T12:02:08Z

astor/code_gen.py

@@ -590,7 +607,13 @@ def recurse(node):
            uni_lit = False  # No formatted byte strings

        else:
-            mystr = node.s
+            if isinstance(node, ast.Str):


You can pass both node and value to this function. This will simplify the code.

@serhiy-storchaka What should I pass in visit_JoinedStr? We don't actually use the value in that branch, but I think it's kind of a smell to pass in a throwaway value in that case.

chrisrink10 · 2018-10-09T00:48:38Z

Thank you @serhiy-storchaka ! I addressed your latest round of comments.

jmfrank63 · 2019-02-11T14:14:29Z

I just commented over at #120 (comment) and could need this.
I am using astor to generate code for python-trio/trio#805
Is there an ETA this PR is getting merged?

berkerpeksag · 2019-02-20T06:27:28Z

@jmfrank63 thank you for the ping. I will try to get this PR in later this week. Sorry for the delay, @chrisrink10!

Kodiologist · 2019-03-20T13:45:24Z

@berkerpeksag Here's another poke. This seems to be a blocker for support for Python 3.8 features in Hy.

chrisrink10 · 2019-03-21T23:03:03Z

@berkerpeksag I fixed the merge conflicts. It would be great if we could get this merged soon. Thanks!

berkerpeksag

LGTM, thank you! Just a minor thing: Could you add a short note to docs/changelog.rst so users can find out this will be in the next release?

chrisrink10 · 2019-03-22T13:44:09Z

Just pushed the changelog update. Thank you!

berkerpeksag · 2019-03-22T14:40:25Z

Sorry, I forgot to say this earlier, but could you add your name to https://github.com/berkerpeksag/astor/blob/master/AUTHORS?

chrisrink10 · 2019-03-22T14:44:18Z

Done!

berkerpeksag · 2019-03-22T14:45:38Z

Thank you and welcome!

Handle new ast.Constant node in SourceGenerator

a764b20

chrisrink10 mentioned this pull request Sep 30, 2018

Change to using ast.Constant rather than ast.Str, ast.Num, etc. basilisp-lang/basilisp#236

Closed

chrisrink10 added 2 commits October 1, 2018 08:25

Test case for ast.Constant node generation

1f5b897

Test cases for the old nodes

b2493a1

chrisrink10 added 2 commits October 1, 2018 20:29

Split out tests per relevant version

4b0a121

Use a helper function for testing

fb65eb1

Test Python 3.8 development builds

99d8752

serhiy-storchaka reviewed Oct 3, 2018

View reviewed changes

Reorder isinstance checks

67916f2

Address PR comments

fbe292f

serhiy-storchaka reviewed Oct 5, 2018

View reviewed changes

chrisrink10 added 2 commits October 5, 2018 07:28

No need to skip tests

8ee206d

Reduce changes to the string handling code

cef9c1c

serhiy-storchaka reviewed Oct 5, 2018

View reviewed changes

Pass node value into string handler

98ca4ad

jmfrank63 mentioned this pull request Feb 18, 2019

Make trio/_core/_run less magical and understandable for static code analysis issue(#542) python-trio/trio#805

Merged

Merge branch 'master' into feature/ast-constants

ab57aa8

berkerpeksag approved these changes Mar 22, 2019

View reviewed changes

Update Changelog

84ec5cb

Update changelog.rst

c5d3271

Chris Rink added 2 commits March 22, 2019 10:41

Add author attribution

3fadadd

Merge branch 'master' into feature/ast-constants

5a1c153

berkerpeksag merged commit 6413903 into berkerpeksag:master Mar 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle new ast.Constant node in SourceGenerator #121

Handle new ast.Constant node in SourceGenerator #121

chrisrink10 commented Sep 30, 2018

berkerpeksag commented Sep 30, 2018

chrisrink10 commented Oct 1, 2018

chrisrink10 commented Oct 2, 2018

berkerpeksag commented Oct 2, 2018

chrisrink10 commented Oct 2, 2018

serhiy-storchaka Oct 3, 2018

serhiy-storchaka commented Oct 3, 2018

chrisrink10 commented Oct 4, 2018

serhiy-storchaka commented Oct 4, 2018

berkerpeksag commented Oct 4, 2018

chrisrink10 commented Oct 4, 2018

serhiy-storchaka left a comment

serhiy-storchaka Oct 5, 2018

serhiy-storchaka Oct 5, 2018

serhiy-storchaka Oct 5, 2018

chrisrink10 commented Oct 5, 2018

serhiy-storchaka Oct 5, 2018

chrisrink10 Oct 5, 2018

serhiy-storchaka Oct 5, 2018

chrisrink10 commented Oct 9, 2018

jmfrank63 commented Feb 11, 2019

berkerpeksag commented Feb 20, 2019

Kodiologist commented Mar 20, 2019

chrisrink10 commented Mar 21, 2019

berkerpeksag left a comment

chrisrink10 commented Mar 22, 2019

berkerpeksag commented Mar 22, 2019

chrisrink10 commented Mar 22, 2019

berkerpeksag commented Mar 22, 2019


		def _handle_string_constant(self, value, precedence, join_fn=None):

Handle new ast.Constant node in SourceGenerator #121

Handle new ast.Constant node in SourceGenerator #121

Conversation

chrisrink10 commented Sep 30, 2018

berkerpeksag commented Sep 30, 2018

chrisrink10 commented Oct 1, 2018

chrisrink10 commented Oct 2, 2018

berkerpeksag commented Oct 2, 2018

chrisrink10 commented Oct 2, 2018

serhiy-storchaka Oct 3, 2018

Choose a reason for hiding this comment

serhiy-storchaka commented Oct 3, 2018

chrisrink10 commented Oct 4, 2018

serhiy-storchaka commented Oct 4, 2018

berkerpeksag commented Oct 4, 2018

chrisrink10 commented Oct 4, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka Oct 5, 2018

Choose a reason for hiding this comment

serhiy-storchaka Oct 5, 2018

Choose a reason for hiding this comment

serhiy-storchaka Oct 5, 2018

Choose a reason for hiding this comment

chrisrink10 commented Oct 5, 2018

serhiy-storchaka Oct 5, 2018

Choose a reason for hiding this comment

chrisrink10 Oct 5, 2018

Choose a reason for hiding this comment

serhiy-storchaka Oct 5, 2018

Choose a reason for hiding this comment

chrisrink10 commented Oct 9, 2018

jmfrank63 commented Feb 11, 2019

berkerpeksag commented Feb 20, 2019

Kodiologist commented Mar 20, 2019

chrisrink10 commented Mar 21, 2019

berkerpeksag left a comment

Choose a reason for hiding this comment

chrisrink10 commented Mar 22, 2019

berkerpeksag commented Mar 22, 2019

chrisrink10 commented Mar 22, 2019

berkerpeksag commented Mar 22, 2019