Skip to content

Conversation

@tausbn
Copy link
Contributor

@tausbn tausbn commented Feb 6, 2025

Once again, the interaction between anchors and extras (specifically comments) was causing trouble.

The root of the problem was the fact that in a[b], we put b in the index field of the subscript node, whereas in a[b,c], we additionally synthesize a Tuple node for b,c (which matches the Python AST).

To fix this, we refactored the grammar slightly so as to make that tuple explicit, such that a subscript node either contains a single expression or the newly added tuple node. This greatly simplifies the logic.

Once again, the interaction between anchors and extras (specifically
comments) was causing trouble.

The root of the problem was the fact that in `a[b]`, we put `b` in the
`index` field of the subscript node, whereas in `a[b,c]`, we
additionally synthesize a `Tuple` node for `b,c` (which matches the
Python AST).

To fix this, we refactored the grammar slightly so as to make that tuple
explicit, such that a subscript node either contains a single expression
or the newly added tuple node. This greatly simplifies the logic.
Copilot AI review requested due to automatic review settings February 6, 2025 14:09
@tausbn tausbn requested a review from a team as a code owner February 6, 2025 14:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff yoff merged commit 37ddaa3 into main Feb 6, 2025
13 checks passed
@yoff yoff deleted the tausbn/python-allow-comments-in-subscripts branch February 6, 2025 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants