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

functemplate: Adapt ast syntax to PEP570 changes on python3.8 #3278

Merged
merged 2 commits into from May 30, 2019

Conversation

Projects
None yet
2 participants
@wisp3rwind
Copy link
Member

commented May 30, 2019

Python 3.8 adds the required posonlyargs to ast.arguments (see the Grammar), resulting in failures such as these: https://travis-ci.org/geigerzaehler/beets-alternatives/jobs/538912903.

There's some discussion about similar breaking API changes (namely in the inspect module) here and here, which however does not mention the lower level ast syntax changes, so I'm assuming that the ast module is supposed to always reflect the grammer such that this is not to be considered a regression.

Does this warrant a changelog entry? Due to the werkzeug incompatibility, Python 3.8 still is not fully supported.

@wisp3rwind wisp3rwind force-pushed the wisp3rwind:fix-ast-py38 branch from 2cd973c to 4ebb118 May 30, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Thanks for pointing this out! It is sort of frustrating that these AST incompatibilities keep cropping up, but I think that's unavoidable.

For a small improvement in legibility/maintainability, what do you think about using a "feature test" instead of a version check to see whether the appropriate field needs to be included? The ast.arguments class has a _fields member that seems to list all its AST children. For example, on Python 3.7:

>>> 'kwonlyargs' in ast.arguments._fields
True
>>> 'posonlyargs' in ast.arguments._fields
False

I think a short changelog entry might be a good idea, even if we need to mention that "real" Python 3.8 compatibility will need to wait for Werkzeug. Especially because Werkzeug is not a "core" dependency (it's only necessary for the web plugin).

@sampsyo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Gave that a try above.

@sampsyo sampsyo merged commit ade1df5 into beetbox:master May 30, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request May 30, 2019

Merge pull request #3278 from wisp3rwind/fix-ast-py38
functemplate: Adapt ast syntax to PEP570 changes on python3.8

sampsyo added a commit that referenced this pull request May 30, 2019

@wisp3rwind

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

Thanks for finishing this up for the release 🎉 Sorry for not replying, I was away from keyboard for a few days...

@wisp3rwind wisp3rwind deleted the wisp3rwind:fix-ast-py38 branch Jun 2, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

No worries at all! Thanks again for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.