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

Issue 341 #458

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

Issue 341 #458

wants to merge 3 commits into from

Conversation

cdonovick
Copy link
Contributor

Summary

resolves #341

Adds _PrecedenceNode as a helper class to automatically parenthesize child nodes when necessary

Test Plan

Adds basic unit tests for implicit parens and extends fuzz testing.

Notes

I had originally tried to allow MaybeSentinel in {l,r}par attrs but this proved difficulty to get right. It would require numerous changes to the parser to ensure proper construction of _BaseParenthesizableNode as they are often explicitly constructed with empty lpar and rpar attributes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 17, 2021
@notEvil
Copy link

notEvil commented Nov 12, 2022

Isn't this pretty important? Afaik any transformer might produce logically different code. Is there a simple "workaround" I don't see?

thank you for libcst!

@cdonovick
Copy link
Contributor Author

@notEvil I have a quick a dirty work around in the linked issue. #341 (comment)

You could probably do something more clever by incorporating the logic I use here in the transformer.

@notEvil
Copy link

notEvil commented Nov 12, 2022

@notEvil I have a quick a dirty work around in the linked issue. #341 (comment)

Thanks, I've seen it but don't want to introduce parentheses.

You could probably do something more clever by incorporating the logic I use here in the transformer.

I just did, except for Power which probably takes me another hour to figure out (because transformer act at child level)*. Still think this PR should get more attention.

* eventually, after I recognized the relevance of wrap_ties

@kiri11
Copy link
Contributor

kiri11 commented Jul 30, 2024

Looks amazing! Prevents tricky issues.

@zsol
Copy link
Member

zsol commented Jul 30, 2024

I'm happy to review this if someone's willing to resurrect and rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Generated Code not equivalent to AST
5 participants