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

Allow no space in doublestar when math operation (#538) (#2095) #2095

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#### _Black_

- `Black` now strips whitespace around `**` operators unless it's only operation in the expression (#2095)

- `Black` now processes one-line docstrings by stripping leading and trailing spaces,
and adding a padding space when needed to break up """". (#1740)

Expand Down
35 changes: 35 additions & 0 deletions src/black/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,7 @@ def f(
for line in transform_line(
current_line, mode=mode, features=split_line_features
):
line = fix_doublestar_in_op(line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little concerned by the performance impact this will have. Doing some rough testing, formatting src/black/__init__.py spends ~6500ms in the format_str method and ~4% (~300ms) of the total runtime was spent in the fix_doublestar_in_op... which is IMO too much for such a simple case like this.

Profiling data (warning: extremely large image)

out

Is it possible to make this more efficient? Cloning the leaf seems to be where most of 300ms cost is coming from. Perhaps moving the logic somewhere into one of the transformers called in transform_line could help since some lines might not need this fixing? I'm sorry for my unadvice, I usually don't review formatting code.

Copy link
Author

Choose a reason for hiding this comment

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

Ohh okay, gonna take a look. Thanks for pointing out!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed with @ichard26, it would be cleaner if it were a StringTransformer like the other transformation. This would additionally solve the performance issue. Few enough lines in Python have DOUBLESTAR tokens that the simplest way to improve performance is just look whether there even is doublestar on the line in do_match().

dst_contents.append(str(line))
return "".join(dst_contents)

Expand Down Expand Up @@ -1844,6 +1845,40 @@ def __bool__(self) -> bool:
return bool(self.leaves or self.comments)


def fix_doublestar_in_op(line: Line) -> Line:
"""Clone line without spaces on doublestar op if is math op."""
leaves: List[Leaf] = []
candidate_rep_count: int = 0
for idx, leaf in enumerate(line.leaves):
new_leaf = leaf.clone()
if new_leaf.type == token.DOUBLESTAR:
if idx > 0 and line.leaves[idx - 1].type in {token.NAME, token.NUMBER}:
new_leaf.prefix = ""
candidate_rep_count += 1
if (
idx > 1
and line.leaves[idx - 1].type == token.DOUBLESTAR
and line.leaves[idx - 2].type in {token.NAME, token.NUMBER}
):
new_leaf.prefix = ""
candidate_rep_count += 1
leaves.append(new_leaf)

if candidate_rep_count <= 2:
leaves = line.leaves

return 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,
)


@dataclass
class EmptyLineTracker:
"""Provides a stateful method that returns the number of potential extra
Expand Down
43 changes: 43 additions & 0 deletions tests/data/doublestar_no_spaces.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#!/usr/bin/env python3.7


def function(**kwargs):
t = a**2 + b**3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should also test what happens if there is a space in the input.

Copy link
Author

@dpalmasan dpalmasan Apr 8, 2021

Choose a reason for hiding this comment

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

Maybe is it more a naming convention for the new option, maybe skip the chosen name is not the best, and I can change it (I assume update to PRs are allowed in code reviews). Regarding to the spaces question, it will yield the same output (meaning, DOUBLESTAR without spaces).

Does your comment mean, update the code and not merge yet, or does it mean: These changes are not convenient, forget about them?

I am sorry if I don't understand correctly it is my first PR on an open-source repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure, but I think Jelle meant you should add test cases which have spaces around the operators, which I think you've done in the code just below! @JelleZijlstra what do you reckon?

I'm not sure about the test writing practices of Black, but I see lots of tests being written a bit more condensed. In your file it's nice to see immediately what's expected from the function name, but another option would be to simply have all the cases in one succession. That's up for debate though!



def function_no_spaces():
return t**2


def function_replace_spaces(**kwargs):
t = a **2 + b** 3 + c ** 4


def function_dont_replace_spaces():
t = t ** 2
{**a, **b, **c}



# output


#!/usr/bin/env python3.7


def function(**kwargs):
t = a**2 + b**3
Copy link
Collaborator

Choose a reason for hiding this comment

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

That I like.



def function_no_spaces():
return t ** 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, looks good.



def function_replace_spaces(**kwargs):
t = a**2 + b**3 + c**4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice.



def function_dont_replace_spaces():
t = t ** 2
{**a, **b, **c}

2 changes: 1 addition & 1 deletion tests/data/expression.diff
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
(~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)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait... what? You really think this is more readable in the new form? I think that once you hit a complex base or exponent, then there is no value in skipping the spaces.

+flags & ~select.EPOLLIN and waiters.write_task is not None
lambda arg: None
lambda a=True: a
Expand Down
2 changes: 1 addition & 1 deletion tests/data/expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ async def f():
-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
2 changes: 1 addition & 1 deletion tests/data/expression_skip_magic_trailing_comma.diff
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
(~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
2 changes: 1 addition & 1 deletion tests/data/pep_572.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
pass
if match := pattern.search(data):
pass
[y := f(x), y ** 2, y ** 3]
[y := f(x), y**2, y**3]
filtered_data = [y for x in data if (y := f(x)) is None]
(y := f(x))
y0 = (y1 := f(x))
Expand Down
9 changes: 9 additions & 0 deletions tests/test_black.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,15 @@ def test_numeric_literals_ignoring_underscores(self) -> None:
black.assert_equivalent(source, actual)
black.assert_stable(source, actual, mode)

@patch("black.dump_to_file", dump_to_stderr)
def test_doublestar_math_op_no_spaces(self) -> None:
source, expected = read_data("doublestar_no_spaces")
mode = replace(DEFAULT_MODE, target_versions=PY36_VERSIONS)
actual = fs(source, mode=mode)
self.assertFormatEqual(expected, actual)
black.assert_equivalent(source, actual)
black.assert_stable(source, actual, mode)
Comment on lines +410 to +417
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anything that would make this formatting Python 3.6+ only. I'm pretty sure using DEFAULT_MODE is fine here, and for such a simple and common test, there's special tooling for it.

SIMPLE_CASES = [
"beginning_backslash",
"bracketmatch",
"cantfit",
"class_blank_parentheses",
"class_methods_new_line",
"collections",
"comments",
"comments2",
"comments3",
"comments4",
"comments5",
"comments6",
"comments7",
"comments_non_breaking_space",
"comment_after_escaped_newline",
"composition",
"composition_no_trailing_comma",
"docstring",
"empty_lines",
"expression",
"fmtonoff",
"fmtonoff2",
"fmtonoff3",
"fmtonoff4",
"fmtskip",
"fmtskip2",
"fmtskip3",
"fmtskip4",
"fmtskip5",
"fstring",
"function",
"function2",
"function_trailing_comma",
"import_spacing",
"long_strings",
"long_strings__edge_case",
"long_strings__regression",
"numeric_literals_py2",
"percent_precedence",
"python2",
"python2_unicode_literals",
"remove_parens",
"slices",
"string_prefixes",
"tricky_unicode_symbols",
"tupleassign",
]

Just add an item of "doublestar_no_spaces" and your test should be picked up!


def test_skip_magic_trailing_comma(self) -> None:
source, _ = read_data("expression.py")
expected, _ = read_data("expression_skip_magic_trailing_comma.diff")
Expand Down