Strip redundant parentheses from assignment exprs #1906
Conversation
src/black/__init__.py
Outdated
| @@ -5481,6 +5478,9 @@ def maybe_make_parens_invisible_in_atom(node: LN, parent: LN) -> bool: | |||
| or is_one_tuple(node) | |||
| or (is_yield(node) and parent.type != syms.expr_stmt) | |||
| or max_delimiter_priority_in_atom(node) >= COMMA_PRIORITY | |||
| or parent.type == syms.expr_stmt | |||
| and parent.children[1].type == token.EQUAL | |||
There was a problem hiding this comment.
I’m not fully sure about this condition
|
Seems to break on pre-3.8. Will fix. EDIT: I’m not sure what these fuzzer errors are. |
|
Alright fuzzer failures seem to be unrelated and they’re failing on HEAD as well. |
|
@JelleZijlstra requesting a review! |
|
Reopening to re-run CI as the fuzz issues should be fixed. |
|
Okay we're hitting #1913 by chance now... |
|
Hm the code I've touched seems unrelated to #1913. Do we wait for that to get resolved first? |
|
Sorry, should've made it clearer that the new unrelated fuzz error was something we've seen before. TR;DR: CI passed for your PR 🎉 |
|
Hi, please let me know if I need to do anything else here. |
There was a problem hiding this comment.
Thanks for working on this! This is my first review of a formatting PR (I handle docs usually) so I hope I'm doing this right and carefully :) This should also be reviewed by Jelle since I believe they are our most qualified reviewer for this sort of stuff. But on with my review first!
Ideally this would have more test cases (complex if and while statements would be nice!), but it isn't a blocker.
I am unsure if children[1] is always guaranteed to be the operator.
The answer is no :(
Here's a few examples of where parent.children[1].type == token.EQUAL falls apart:
test += (a := 1)(who would write this tho?)a, b = (test := (1, 2))test: int = (test2 := 2)
edit: don't ask me how to workaround this problem, I have no idea
Also, you'll need to edit this section adding a note how Black will not remove parentheses where an assignment expression is used since it's invalid.
|
Alright, I think it should be correct now. But perhaps we should wait for the refactoring that’s going on in #1958 to finish. @jayaddison Are you planning to touch this code? |
There was a problem hiding this comment.
Also, you'll need to edit this section adding a note how Black will not remove parentheses where an assignment expression is used since it's invalid.
This was never resolved, please do so.
But overall, great improvement, thanks for your time and effort! If you please, add yourself to the AUTHORS list in the readme.
@JelleZijlstra, I would appreciate if you could make sure I haven't missed anything vital! This PR should be mostly ready, just a few documentation issues (of which I can handle and review). |
You've got some experience at least compared to me :)
I was about to comment about that, don't fret too much as now I do see how you might have skipped over it since it was poorly worded. I should really reread stuff I write before pressing "comment". Ugh, old habits die hard. |
|
@ichard26 don't blame yourself, it's my bad! You clearly requested changes, so I should have noticed. |
Isn’t this implicit, since that section in the docs talks about “optional parentheses”? The parentheses as used in something like Also I would expect that behavior from the principle of least surprise, we should never be generating invalid code in the first place :P |
|
@xrisk yeah you're right lol. Apologies for the noise. Thanks for the PR! |
#1656
This converts code of the form:
into
which is consistent with how
if (foo):is transformed intoif foo:and also with examples in PEP 572.—
Although all tests are passing, please review particularly this condition:
and parent.children[1].type == token.EQUALI am unsure if children[1] is always guaranteed to be the operator.