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

Hug power operators if its operands are "simple" #2726

Merged
merged 12 commits into from Jan 25, 2022
1 change: 1 addition & 0 deletions CHANGES.md
Expand Up @@ -17,6 +17,7 @@
- Tuple unpacking on `return` and `yield` constructs now implies 3.8+ (#2700)
- Unparenthesized tuples on annotated assignments (e.g
`values: Tuple[int, ...] = 1, 2, 3`) now implies 3.8+ (#2708)
- Remove spaces around power operators if both operands are simple (#2726)

### Packaging

Expand Down
6 changes: 6 additions & 0 deletions docs/the_black_code_style/current_style.md
Expand Up @@ -291,6 +291,12 @@ multiple lines. This is so that _Black_ is compliant with the recent changes in
[PEP 8](https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator)
style guide, which emphasizes that this approach improves readability.

Almost all operators will be surrounded by single spaces, the only exceptions are unary
operators (`+`, `-`, and `~`), and power operators when both operands are simple.
For power ops, an operand is considered simple if it's only a NAME, numeric
CONSTANT, or attribute access (chained attribute access is allowed), with or without a
preceding unary operator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reading it through once more, we could also say that the surrounding spaces are removed for unaries and simple powers. Now it just says that they are an exception to the rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a little confused, could you elaborate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry, I missed this: I meant to say that we don't actually say what happens to unaries and powers. We only say that they are an exception to the rule. Maybe something like: "[the only exceptions are unaries and simple pows], for which the spaces are removed."


### Slices

PEP 8
Expand Down
22 changes: 17 additions & 5 deletions src/black/linegen.py
Expand Up @@ -22,7 +22,12 @@
from black.strings import get_string_prefix, fix_docstring
from black.strings import normalize_string_prefix, normalize_string_quotes
from black.trans import Transformer, CannotTransform, StringMerger
from black.trans import StringSplitter, StringParenWrapper, StringParenStripper
from black.trans import (
StringSplitter,
StringParenWrapper,
StringParenStripper,
hug_power_op,
)
ichard26 marked this conversation as resolved.
Show resolved Hide resolved
from black.mode import Mode
from black.mode import Feature

Expand Down Expand Up @@ -342,9 +347,9 @@ def transform_line(
):
# Only apply basic string preprocessing, since lines shouldn't be split here.
if mode.experimental_string_processing:
transformers = [string_merge, string_paren_strip]
transformers = [string_merge, string_paren_strip, hug_power_op]
else:
transformers = []
transformers = [hug_power_op]
elif line.is_def:
transformers = [left_hand_split]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should hug_power_op be in here? Maybe it's easier to just unconditionally add it at the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah I forgot that power ops can exist as parameter default expressions, gah. Good catch!

else:
Expand Down Expand Up @@ -394,6 +399,7 @@ def _rhs(
standalone_comment_split,
string_paren_wrap,
rhs,
hug_power_op,
]
else:
transformers = [
Expand All @@ -402,12 +408,18 @@ def _rhs(
string_split,
string_paren_wrap,
rhs,
hug_power_op,
]
else:
if line.inside_brackets:
transformers = [delimiter_split, standalone_comment_split, rhs]
transformers = [
delimiter_split,
standalone_comment_split,
rhs,
hug_power_op,
]
else:
transformers = [rhs]
transformers = [rhs, hug_power_op]

for transform in transformers:
# We are accumulating lines in `result` because we might want to abort
Expand Down
86 changes: 84 additions & 2 deletions src/black/trans.py
Expand Up @@ -24,9 +24,9 @@
import sys

if sys.version_info < (3, 8):
from typing_extensions import Final
from typing_extensions import Literal, Final
else:
from typing import Final
from typing import Literal, Final

from mypy_extensions import trait

Expand Down Expand Up @@ -71,6 +71,88 @@ def TErr(err_msg: str) -> Err[CannotTransform]:
return Err(cant_transform)


def hug_power_op(line: Line, features: Collection[Feature]) -> Iterator[Line]:
"""A transformer which normalizes spacing around power operators."""

# Performance optimization to avoid unnecessary Leaf clones and other ops.
for leaf in line.leaves:
if leaf.type == token.DOUBLESTAR:
break
else:
raise CannotTransform("No doublestar token was found in the line.")

def is_simple_lookup(index: int, step: Literal[1, -1]) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to add line as a parameter? I'm thinking creating functions every time a power is formatted could be more costly than passing an additional parameter, but honestly I'm not sure how impactful it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So for some reason I'm starting to really like nested functions as part of larger functions. The nested functions aren't intended for generic reuse (for now) but are logically a bit more separate if that makes sense. I'm happy to change this bit as I understand this may be not a style most would like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, my intuition would be to make private functions in that case. But style-wise I think it's a matter of taste, I was just more concerned about the potential performance hit with math-heavy code. Haven't tested it at all though 😄

# Brackets and parenthesises indicate calls, subscripts, etc. ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Brackets and parenthesises indicate calls, subscripts, etc. ...
# Brackets and parentheses indicate calls, subscripts, etc. ...

# basically stuff that doesn't count as "simple". Only a NAME lookup
# or dotted lookup (eg. NAME.NAME) is OK.
if step == -1:
disallowed = {token.RPAR, token.RSQB}
else:
disallowed = {token.LPAR, token.LSQB}

while 0 <= index < len(line.leaves):
current = line.leaves[index]
if current.type in disallowed:
return False
if current.type not in {token.NAME, token.DOT} or current.value == "for":
# If the current token isn't disallowed, we'll assume this is simple as
# only the disallowed tokens are semantically attached to this lookup
# expression we're checking. Also, stop early if we hit the 'for' bit
# of a comprehension.
return True

index += step

return True

def is_simple_operand(index: int, kind: Literal["base", "exponent"]) -> bool:
# An operand is considered "simple" if's a NAME, a numeric CONSTANT, a simple
# lookup (see above), with or without a preceding unary operator.
start = line.leaves[index]
if start.type in {token.NAME, token.NUMBER}:
return is_simple_lookup(index, step=(1 if kind == "exponent" else -1))

if start.type in {token.PLUS, token.MINUS, token.TILDE}:
if line.leaves[index + 1].type in {token.NAME, token.NUMBER}:
# step is always one as bases with a preceding unary op will be checked
# for simplicity starting from the next token (so it'll hit the check
# above).
return is_simple_lookup(index + 1, step=1)

return False

leaves: List[Leaf] = []
should_hug = False
for idx, leaf in enumerate(line.leaves):
new_leaf = leaf.clone()
if should_hug:
new_leaf.prefix = ""
should_hug = False

should_hug = (
(0 < idx < len(line.leaves) - 1)
and leaf.type == token.DOUBLESTAR
and is_simple_operand(idx - 1, kind="base")
and line.leaves[idx - 1].value != "lambda"
and is_simple_operand(idx + 1, kind="exponent")
)
if should_hug:
new_leaf.prefix = ""

leaves.append(new_leaf)

yield Line(
mode=line.mode,
depth=line.depth,
leaves=leaves,
comments=line.comments,
bracket_tracker=line.bracket_tracker,
inside_brackets=line.inside_brackets,
should_split_rhs=line.should_split_rhs,
magic_trailing_comma=line.magic_trailing_comma,
)


class StringTransformer(ABC):
"""
An implementation of the Transformer protocol that relies on its
Expand Down
2 changes: 1 addition & 1 deletion src/black_primer/primer.json
Expand Up @@ -81,7 +81,7 @@
},
"flake8-bugbear": {
"cli_arguments": ["--experimental-string-processing"],
"expect_formatting_changes": false,
"expect_formatting_changes": true,
"git_clone_url": "https://github.com/PyCQA/flake8-bugbear.git",
"long_checkout": false,
"py_versions": ["all"]
Expand Down
44 changes: 30 additions & 14 deletions tests/data/expression.diff
Expand Up @@ -11,15 +11,25 @@
True
False
1
@@ -29,63 +29,96 @@
@@ -21,71 +21,104 @@
Name1 or (Name2 and Name3) or Name4
Name1 or Name2 and Name3 or Name4
v1 << 2
1 >> v2
1 % finished
-1 + v2 - v3 * 4 ^ 5 ** v6 / 7 // 8
-((1 + v2) - (v3 * 4)) ^ (((5 ** v6) / 7) // 8)
+1 + v2 - v3 * 4 ^ 5**v6 / 7 // 8
+((1 + v2) - (v3 * 4)) ^ (((5**v6) / 7) // 8)
not great
~great
+value
-1
~int and not v1 ^ 123 + v2 | True
(~int) and (not ((v1 ^ (123 + v2)) | True))
-+really ** -confusing ** ~operator ** -precedence
-flags & ~ select.EPOLLIN and waiters.write_task is not None
++(really ** -(confusing ** ~(operator ** -precedence)))
++(really ** -(confusing ** ~(operator**-precedence)))
+flags & ~select.EPOLLIN and waiters.write_task is not None
lambda arg: None
lambda a=True: a
Expand Down Expand Up @@ -88,15 +98,19 @@
+ *more,
+]
{i for i in (1, 2, 3)}
{(i ** 2) for i in (1, 2, 3)}
-{(i ** 2) for i in (1, 2, 3)}
-{(i ** 2) for i, _ in ((1, 'a'), (2, 'b'), (3, 'c'))}
+{(i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))}
{((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)}
-{((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)}
+{(i**2) for i in (1, 2, 3)}
+{(i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))}
+{((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3)}
[i for i in (1, 2, 3)]
[(i ** 2) for i in (1, 2, 3)]
-[(i ** 2) for i in (1, 2, 3)]
-[(i ** 2) for i, _ in ((1, 'a'), (2, 'b'), (3, 'c'))]
+[(i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))]
[((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)]
-[((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)]
+[(i**2) for i in (1, 2, 3)]
+[(i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))]
+[((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3)]
{i: 0 for i in (1, 2, 3)}
-{i: j for i, j in ((1, 'a'), (2, 'b'), (3, 'c'))}
+{i: j for i, j in ((1, "a"), (2, "b"), (3, "c"))}
Expand Down Expand Up @@ -181,10 +195,12 @@
SomeName
(Good, Bad, Ugly)
(i for i in (1, 2, 3))
((i ** 2) for i in (1, 2, 3))
-((i ** 2) for i in (1, 2, 3))
-((i ** 2) for i, _ in ((1, 'a'), (2, 'b'), (3, 'c')))
+((i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c")))
(((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3))
-(((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3))
+((i**2) for i in (1, 2, 3))
+((i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c")))
+(((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3))
(*starred,)
-{"id": "1","type": "type","started_at": now(),"ended_at": now() + timedelta(days=10),"priority": 1,"import_session_id": 1,**kwargs}
+{
Expand Down Expand Up @@ -403,13 +419,13 @@
+ return True
+if (
+ ~aaaa.a + aaaa.b - aaaa.c * aaaa.d / aaaa.e
+ | aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l ** aaaa.m // aaaa.n
+ | aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l**aaaa.m // aaaa.n
+):
+ return True
+if (
+ ~aaaaaaaa.a + aaaaaaaa.b - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e
+ | aaaaaaaa.f & aaaaaaaa.g % aaaaaaaa.h
+ ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l ** aaaaaaaa.m // aaaaaaaa.n
+ ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n
+):
+ return True
+if (
Expand All @@ -419,7 +435,7 @@
+ | aaaaaaaaaaaaaaaa.f & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h
+ ^ aaaaaaaaaaaaaaaa.i
+ << aaaaaaaaaaaaaaaa.k
+ >> aaaaaaaaaaaaaaaa.l ** aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n
+ >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n
+):
+ return True
+(
Expand Down
30 changes: 15 additions & 15 deletions tests/data/expression.py
Expand Up @@ -282,15 +282,15 @@ async def f():
v1 << 2
1 >> v2
1 % finished
1 + v2 - v3 * 4 ^ 5 ** v6 / 7 // 8
((1 + v2) - (v3 * 4)) ^ (((5 ** v6) / 7) // 8)
1 + v2 - v3 * 4 ^ 5**v6 / 7 // 8
((1 + v2) - (v3 * 4)) ^ (((5**v6) / 7) // 8)
not great
~great
+value
-1
~int and not v1 ^ 123 + v2 | True
(~int) and (not ((v1 ^ (123 + v2)) | True))
+(really ** -(confusing ** ~(operator ** -precedence)))
+(really ** -(confusing ** ~(operator**-precedence)))
flags & ~select.EPOLLIN and waiters.write_task is not None
lambda arg: None
lambda a=True: a
Expand Down Expand Up @@ -347,13 +347,13 @@ async def f():
*more,
]
{i for i in (1, 2, 3)}
{(i ** 2) for i in (1, 2, 3)}
{(i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))}
{((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)}
{(i**2) for i in (1, 2, 3)}
{(i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))}
{((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3)}
[i for i in (1, 2, 3)]
[(i ** 2) for i in (1, 2, 3)]
[(i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))]
[((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3)]
[(i**2) for i in (1, 2, 3)]
[(i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c"))]
[((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3)]
{i: 0 for i in (1, 2, 3)}
{i: j for i, j in ((1, "a"), (2, "b"), (3, "c"))}
{a: b * 2 for a, b in dictionary.items()}
Expand Down Expand Up @@ -441,9 +441,9 @@ async def f():
SomeName
(Good, Bad, Ugly)
(i for i in (1, 2, 3))
((i ** 2) for i in (1, 2, 3))
((i ** 2) for i, _ in ((1, "a"), (2, "b"), (3, "c")))
(((i ** 2) + j) for i in (1, 2, 3) for j in (1, 2, 3))
((i**2) for i in (1, 2, 3))
((i**2) for i, _ in ((1, "a"), (2, "b"), (3, "c")))
(((i**2) + j) for i in (1, 2, 3) for j in (1, 2, 3))
(*starred,)
{
"id": "1",
Expand Down Expand Up @@ -588,13 +588,13 @@ async def f():
return True
if (
~aaaa.a + aaaa.b - aaaa.c * aaaa.d / aaaa.e
| aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l ** aaaa.m // aaaa.n
| aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l**aaaa.m // aaaa.n
):
return True
if (
~aaaaaaaa.a + aaaaaaaa.b - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e
| aaaaaaaa.f & aaaaaaaa.g % aaaaaaaa.h
^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l ** aaaaaaaa.m // aaaaaaaa.n
^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n
):
return True
if (
Expand All @@ -604,7 +604,7 @@ async def f():
| aaaaaaaaaaaaaaaa.f & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h
^ aaaaaaaaaaaaaaaa.i
<< aaaaaaaaaaaaaaaa.k
>> aaaaaaaaaaaaaaaa.l ** aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n
>> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n
):
return True
(
Expand Down