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

Use parentheses in: a = b == c #449

Closed
njsmith opened this issue Aug 14, 2018 · 14 comments
Closed

Use parentheses in: a = b == c #449

njsmith opened this issue Aug 14, 2018 · 14 comments
Labels
F: parentheses Too many parentheses, not enough parentheses, and so on. T: style What do we want Blackened code to look like?

Comments

@njsmith
Copy link

njsmith commented Aug 14, 2018

Some changes black wants to make to my code:

-on_windows = (os.name == "nt")
+on_windows = os.name == "nt"
-        currently_registered = (fd in self._registered)
+        currently_registered = fd in self._registered

The parentheses were originally there to aid readability, by emphasizing that RHS of the = is an unusual-but-valid self-contained expression and not, like, a chained assignment or comprehension or something. Perhaps black should preserve, or even add, parentheses in cases like this? I guess the in case is more debateable, but the = == case is really really easy to misread.

@zsol zsol added T: enhancement New feature or request F: parentheses Too many parentheses, not enough parentheses, and so on. labels Aug 15, 2018
@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

This is one of those "eye of the beholder" cases. For each careful Nathaniel pair of readability parens, there's going to be plenty of unnecessary ones put by less experienced developers.

If I make the exception for == alone, and in assignment alone, this feels icky to me as it requires another sentence in the docs explaining that particular edge case.

FWIW I consider the latter change preferable. in is a bit overloaded in Python, sure, but the parens-free version reads like English to me. We wouldn't put parens around:

if leaf.type in OPENING_BRACKETS:

For how small Black's codebase is, we have quite a few counter-examples there:

is_suite_like = node.parent and node.parent.type in STATEMENT

and so on :-)

I'm not closing this one just yet but I feel this is one of those cases where we can find a large amount of examples both in favor and against parentheses.

@ambv ambv added T: style What do we want Blackened code to look like? and removed T: enhancement New feature or request labels Aug 17, 2018
@njsmith
Copy link
Author

njsmith commented Aug 17, 2018

I also noticed myself writing a = (b or c) in some code last night, which... sigh, yeah, I do prefer that but I don't know why and probably won't convince you, so I guess I'll have to wish it a fond farewell. (Probably the rule is "is this a test expression that I'd expect to only see inside an 'if', but now I'm doing a weird thing and reifying the test result as a value"? Something like that.)

However, I will still argue that you really really should distinguish these cases:

name1 = name2 = name3

name1 = name2 == name3

name1 == name2 == name3

They get three radically different parses. The top and the bottom are ok on their own – they appear in different contexts, so you're not likely to mistake one for the other, and they're internally consistent. But the one in the middle is one character off from both of the other two, more complicated, and much much rarer, so it's easy for your eye to try to autocorrect it to one of the others. Parentheses fix all that.

So, here's an concrete proposal: when the top node of the AST for the RHS of an assignment is in the set: ==, <=, >=, <, >, !=, then surround the RHS with parentheses to make sure it's not confusable with chained comparison or chained assignment.

I guess there's an argument for throwing '(not) is' and '(not) in' into the set that trigger this, so that it matches the set of chainable comparisons. But that's more an argument based on like, spec simplicity than that anyone is really going to think that a = b in c is a chained comparison a == b in c. (I bet 97% of python programmers would be surprised by how that last expression is evaluated.)

@ambv
Copy link
Collaborator

ambv commented Sep 26, 2018

I'm coming back to this now. Would you consider assignments special here? What if an assignment it augmented with a type annotation? What about keyword arguments?

@njsmith
Copy link
Author

njsmith commented Sep 27, 2018

For concreteness, I guess you're asking about other uses of the = character like:

on_windows: bool = os.name == "nt"

check_sockets(on_windows=os.name == "nt")

or, for that matter,

match_count += new_value == old_value

if on_windows := os.name == "nt":

I don't think I've ever written any of these, so I don't have a strong intuition. They do look pretty strange? But at least in these cases there's already some other hint to break the visual similarity with a == b == c, like extra characters or the missing spaces around the kwarg =, so the benefits aren't as unambiguous. The := example strikes me as particularly terrible, but I'm not sure if that's because := makes my eyes hurt in general or more specifically because := seems pretty confusable with == when skimming.

Of course for black's purposes, we also need a rule that's simple, unambiguous, implementable, explainable. So my two suggestions would be:

More aggressive, what I more or less do currently: put parentheses around any Compare node that's not the immediate child of if, while, and, or. Result: this would add parentheses to all of the above examples.

Less aggressive: put parentheses around any Compare node that's the right-hand side of an assignment. For simplicity, apply this to all forms of assignment. Result: this would add parentheses to everything above, except for the kwarg.

@ambv
Copy link
Collaborator

ambv commented Nov 6, 2018

I'm not so sure about this. It feels very subtle to me when this would be considered helpful versus noisy. An additional rule for handling parentheses in some special case would make it surprising to some users, my intuition is that for every Nathaniel happy with the change, we would get a Joe who would complain at the surprising change.

I see what you're after here, @njsmith, and I don't disagree. I just need to feel that this will be easy enough to explain to beginners.

@rknLA
Copy link

rknLA commented Aug 1, 2019

I would like to add a strong +1 to the original request for keeping (or introducing) parens in the equality check, a = b == c, case.

I am just now considering using black for a project, and this one alone is borderline deal-breaker for me, primarily for the following reasons:

  1. Equality check and assignment operations use the same character,
  2. foo = bar = baz and foo = bar == baz are both equality valid python
  3. It is dramatically easier to see the intention of foo = (bar == baz) than foo = bar == baz.

The main readability issue for me is, without the parents, it triggers an instinctive double-take when reading code, which hampers one's ability to skim through a function. It introduces a pause, "wait, what is that line actually doing?", which is easily mitigated with parens.

In the simple a = b == c case, I think it's pretty easy to see, but when your lines look something like (but with more real-world values):

some_flag = a_configuration_variable == "known_constant"

then it's not as immediately clear that this is not a triple-assignment.

Seeing as one concern is explanation to beginners, looking at the documentation, this situation is already currently ambiguous:

Please note that Black does not add or remove any additional nested parentheses that you might want to have for clarity or further code organization. For example those parentheses are not going to be removed:

return not (this or that)
decision = (maybe.this() and values > 0) or (maybe.that() and values < 0)

IMO, if I've deliberately written a = (b == c), then this is roughly equivalent to the nested parentheses case.

The docs could also be written as (additions in bold):

Parentheses

Some parentheses are optional in the Python grammar. Any expression can be wrapped in a pair of parentheses to form an atom. There are a few interesting cases:

  • if (...):
  • while (...):
  • for (...) in (...):
  • assert (...), (...)
  • from X import (...)
  • assignments like:
    • target = (...)
    • target: type = (...)
    • some, *un, packing = (...)
    • augmented += (...)

In those cases, parentheses are removed when the entire statement fits in one line, or if the inner expression doesn't have any delimiters to further split on, and if the statement is not an assignment to an equality check. If there is only a single delimiter and the expression starts or ends with a bracket, the parenthesis can also be successfully omitted since the existing bracket pair will organize the expression neatly anyway. Otherwise, the parentheses are added.

Please note that Black does not add or remove any additional nested parentheses that you might want to have for clarity or further code organization. For example those parentheses are not going to be removed:

return not (this or that)
decision = (maybe.this() and values > 0) or (maybe.that() and values < 0)

For readability, Black will add parentheses to simple equality-check-assignment operations:

target = (another == variable)

@ambv ambv added the stable label Mar 3, 2020
@ambv ambv added this to To Do in Stable release via automation Mar 3, 2020
@ambv
Copy link
Collaborator

ambv commented Mar 3, 2020

OK, let's do it.

@pradyunsg
Copy link
Contributor

Another test case from pip's codebase:

class Foo:
    def _get_candidates(self, link, canonical_package_name):
        # type: (Link, str) -> List[Any]
        can_not_cache = (
            not self.cache_dir or
            not canonical_package_name or
            not link
        )
        # <more code here>

Black's output:

class Foo:
    def _get_candidates(self, link, canonical_package_name):
        # type: (Link, str) -> List[Any]
        can_not_cache = not self.cache_dir or not canonical_package_name or not link
        # <more code here>

Possibly nicer output:

class Foo:
    def _get_candidates(self, link, canonical_package_name):
        # type: (Link, str) -> List[Any]
        can_not_cache = (not self.cache_dir or not canonical_package_name or not link)
        # <more code here>

My workaround for now, is gonna be to add a small comment and make black break the line as in the first example.

@JelleZijlstra
Copy link
Collaborator

@pradyunsg I'm not sure why that case particularly needs parentheses. It's not nearly as visually confusing as the examples with == cited above.

@JelleZijlstra
Copy link
Collaborator

I now believe we shouldn't make any change here, though I won't object if there's consensus in favor of the change. @Shivansh-007's implementation makes it possible to see the effect of the change on real-world code. This shows:

  • The pattern is pretty common, with seven changes in Black's own codebase.
  • Many cases have complicated expressions on the LHS of the ==, like is_last_column = len(columns) - 1 == i. I don't think the extra parens are valuable there.
  • Even in cases where the LHS is just a simple variable, I don't personally find code like a = b == c very confusing. The number of thumbs-down reactions on this issue suggests I'm not the only one.

@Shivansh-007
Copy link
Contributor

Shivansh-007 commented Jan 17, 2022

I agree with what @felix-hilden had posted on the PR

A good middle ground could be using a definition of "complex" like in our power hugging endeavour. It's not so easy to determine if something can be used as an assignment target, but a combination of name-attribute-subscript is a good approximation.

For those who aren't familiar with the power hugging endeavour, it corresponds to #2726.

This can be seen in the changes in primer also, the following changes are helping in improving the readability and are comparisons of "simple" nodes.

A node would be considered "simple" if it's only a NAME, numeric CONSTANT, or attribute access (chained attribute access is allowed).

-        left_ea = blk_vals.ndim == 1
+        left_ea = (blk_vals.ndim == 1)

-        result = arr == other
+        result = (arr == other)

-        actual = self.index.values == self.index
+        actual = (self.index.values == self.index)

Whereas, places where the nodes are a bit "complex", I am finding it better without the parentheses:

-        result = Series(a1, dtype=cat_type) == Series(a2, dtype=cat_type)
+        result = (Series(a1, dtype=cat_type) == Series(a2, dtype=cat_type))

-                mask &= (self._ascending_count - start) % step == 0
+                mask &= ((self._ascending_count - start) % step == 0)

-    debug = env.get(str("_VIRTUALENV_PERIODIC_UPDATE_INLINE")) == str("1")
+    debug = (env.get(str("_VIRTUALENV_PERIODIC_UPDATE_INLINE")) == str("1"))

Partially a crosspost of #2770 (comment)

Shivansh-007 added a commit to Shivansh-007/black that referenced this issue Jan 20, 2022
@felix-hilden
Copy link
Collaborator

felix-hilden commented Jan 31, 2022

As this seems a bit controversial, maybe we should instead try to respect user-inserted parentheses, like in math expressions and other cases like in a comment above already:

a = (2 * 2) + 1  # unchanged, even though it's clearly optional
a = (b == c) == d  # unchanged, even though optional
a = (b == c)  # could be unchanged

This clashes with our want to not respect previous formatting though. So I'm not sure if it's worth it. But I thought I'd open up a discussion about it.

@Waxxx333
Copy link

I'd really like if removal of parenthesis was optional. Something in the config to either enable or disable removal of parenthesis.

@hauntsaninja
Copy link
Collaborator

Looking at the results in #2770 , I prefer never adding the parentheses over always adding the parentheses (and given the number of thumbs down on OP, the status quo is certainly safer here)

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: parentheses Too many parentheses, not enough parentheses, and so on. T: style What do we want Blackened code to look like?
Projects
No open projects
Stable release
  
To Do (nice to have)
Development

Successfully merging a pull request may close this issue.

10 participants