-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Handle right parens in join comma builder #5711
Conversation
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
) | ||
|
||
func_with_bad_parens_that_wont_fit_in_one_line( | ||
"short string that should have parens stripped", x, y, z | ||
("short string that should have parens stripped"), x, y, z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this locally, black preserves the parentheses, regardless of what the text says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can open a black bug :P
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
) | ||
|
||
func_with_bad_parens_that_wont_fit_in_one_line( | ||
"short string that should have parens stripped", x, y, z | ||
("short string that should have parens stripped"), x, y, z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can open a black bug :P
Summary
The
JoinCommaSeparatedBuilder
only tested if the first token after the node is a,
.However, this always fails for expressions with parentheses, because the parentheses are not part of the expression's range.
This PR changes the
JoinCommaSeparatedBuilder
to skip right parens. However, we need to be careful, becausenested(call(arg),)
only the outer call has a trailing commain this example, not the inner arg. The way this is implemented is by also passing an approxiamted end of the sequence (normally the end of the enclosing node, but we don't always know precisely), and only search between the end of the last node and the end of the sequence.
I further went along and also fixed an issue where single argument call expressions were always parenthesized because
is_expression_parenthesized
doesn't know that the parens from the arguments belong to the call. The way to solve this is by handling parenthesesmanually inside of
FormatCallExpr
if the call only has a single argument.Test Plan
See updated snapshot tests.