-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe is it more a naming convention for the new option, maybe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That I like. |
||
|
||
|
||
def function_no_spaces(): | ||
return t ** 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Lines 16 to 63 in 6d0bdc2
Just add an item of |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def test_skip_magic_trailing_comma(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
source, _ = read_data("expression.py") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expected, _ = read_data("expression_skip_magic_trailing_comma.diff") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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'm a little concerned by the performance impact this will have. Doing some rough testing, formatting
src/black/__init__.py
spends ~6500ms in theformat_str
method and ~4% (~300ms) of the total runtime was spent in thefix_doublestar_in_op
... which is IMO too much for such a simple case like this.Profiling data (warning: extremely large image)
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.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.
Ohh okay, gonna take a look. Thanks for pointing out!
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.
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()
.