Skip to content

fix: with item precedence for multiple context managers with brackets #181

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

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

Hellebore
Copy link
Contributor

@Hellebore Hellebore commented Nov 16, 2022

Currently when parsing code like the following:

with (open('d') as d,
      open('e') as e):
  f

tree-sitter is converting it into a with_statement which has one with_item, which contains a tuple of as_pattern.

In the Python specification multiple context managers should each be represented by a WithItem, each of which has a single (possibly aliased) context manager.

This was causing problems for us when using tree-sitter to parse such structures into an AST.

This PR adjusts (correctly I hope, though open to being wrong!) the precedence of the with_item to fix this issue and bring it into line with the Python AST.

@Hellebore Hellebore marked this pull request as draft November 17, 2022 12:07
@Hellebore Hellebore marked this pull request as ready for review November 18, 2022 09:25
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable. 👍

I was curious to see how the "official" representation of multiple context handlers is for Python 2 (as the ASTs there do not have a sequence of with_items). Turns out Python 2 just unfolds it into a series of nested with statements.

@tausbn
Copy link
Contributor

tausbn commented Dec 7, 2022

No other objections, it seems, so I'll go ahead and merge this.

Thanks for the fix!

@tausbn tausbn merged commit 1784048 into tree-sitter:master Dec 7, 2022
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.

2 participants