Skip to content

Allow user to specify version_info #27

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dbieber
Copy link

@dbieber dbieber commented Sep 6, 2018

Allow user to specify version_info rather than assuming AST version matches the current Python version. This is useful if the user is using e.g. gast to get a different AST version from the Python version they are using.

…atches the current Python version. This is useful if the user is using e.g. gast to get a different AST version from the Python version they are using.
@dbieber
Copy link
Author

dbieber commented Oct 1, 2018

Rebased with the latest commits.

Copy link

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

+1 for the feature. Ideally, would allow to specify it via command line flag to __main__.py too.

The code could use some cleanup.

@@ -24,14 +24,15 @@ def interleave(inter, f, seq):
inter()
f(x)

class Unparser:
class Unparser(object):
Copy link

Choose a reason for hiding this comment

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

This is apparently change unrelated to the functionality provided.

@@ -770,6 +771,11 @@ def _arguments(self, t):
if t.kwarg.annotation:
self.write(": ")
self.dispatch(t.kwarg.annotation)
elif hasattr(t.kwarg, 'id'):
self.write("**"+t.kwarg.id)
Copy link

Choose a reason for hiding this comment

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

Spaces around + ?

Copy link
Author

Choose a reason for hiding this comment

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

While I agree that would be better style, it's better to remain consistent with the rest of the file.
Other commits can improve the style globally if it's desired.

Copy link

Choose a reason for hiding this comment

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

it's better to remain consistent with the rest of the file.

Absolutely! I actually went and check how unparser.py has it, and in immediate vicinity of your changes I saw this: https://github.com/simonpercivall/astunparse/blob/master/lib/astunparse/unparser.py#L763 , that's why I made a comment. Looking in more detail, I see cases like https://github.com/simonpercivall/astunparse/blob/master/lib/astunparse/unparser.py#L804 , https://github.com/simonpercivall/astunparse/blob/master/lib/astunparse/unparser.py#L809 - so yes, your change looks good wrt to them.

Other commits can improve the style globally if it's desired.

I'm sure that falls on maintainer's plate. (I'm just a random passer-by who spotted an issue with the lib: #39, and proceeded to check how active maintenance of this repo by looking at pending PRs, then figuring I could benefit from this PR too. Oh, btw, ping @simonpercivall ;-) ).

Copy link
Owner

Choose a reason for hiding this comment

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

Off-topic: I'm pretty bad at maintaining this repository :/

Re. style: The style (and code) line-by-line should be kept as near identical as possible to the different versions of Tools/parser/unparse.py in the official cpython source tree.

@dbieber
Copy link
Author

dbieber commented Jan 15, 2020

Resolved conflicts.
Still interested in getting this checked in if @simonpercivall will have it.

Off-topic: I'm pretty bad at maintaining this repository :/

No worries :)

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