Update Black tests #20794
Update Black tests #20794
Conversation
ntBre
added
internal
|
ntBre
force-pushed
the
brent/import-black-tests
branch
from
962fb05 to
a48d96d
Compare
| ] # comment | ||
| ``` | ||
|
|
||
| ## Black Differences |
There was a problem hiding this comment.
These look like known deviations since we don't implement the remove_lone_list_item_parens style yet.
| def trailing_comma2[T=int](a: str,): | ||
| pass | ||
| def weird_syntax[T=lambda: 42, **P=lambda: 43, *Ts=lambda: 44](): pass |
There was a problem hiding this comment.
This is the only new code in this file, I think we've gotten closer to black here, if I'm interpreting the diff correctly.
| -this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = ( | ||
| - function() | ||
| -) | ||
| +this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = function() |
There was a problem hiding this comment.
These all look like improvements to me, but I'm slightly confused about this case and the one above without comments. Maybe we have a slightly different line-length metric from black, I'm not sure why they wrap these.
There was a problem hiding this comment.
After looking around a bit more, I think this is related to #11820.
| -def foo(): return "mock" # fmt: skip | ||
| -if True: print("yay") # fmt: skip | ||
| -for i in range(10): print(i) # fmt: skip |
| -b = [c for c in "A very long string that would normally generate some kind of collapse, since it is this long"] # fmt: skip | ||
| +b = [c for c in "A very long string that would normally generate some kind of collapse, since it is this long"] # fmt: skip |
There was a problem hiding this comment.
I'm less sure about this case and the j = 1 case above. It looks like we're adding a space before the # fmt: skip. It seems like we should probably skip that too.
There was a problem hiding this comment.
Yes, we format the fmt: skip comment. I don't think this is something that we need to change.
| # Regressed again by https://github.com/psf/black/pull/4498 | ||
| -s = ( | ||
| - "With single quote: ' " | ||
| - f" {my_dict['foo']}" | ||
| - ' With double quote: " ' | ||
| - f' {my_dict["bar"]}' | ||
| -) | ||
| +s = f"With single quote: ' {my_dict['foo']} With double quote: \" {my_dict['bar']}" |
There was a problem hiding this comment.
I think this is expected and in line with the string_processing preview style.
There was a problem hiding this comment.
These should be expected in line with multiline_string_handling being a non-goal.
There was a problem hiding this comment.
These seem expected, more lone_list_item changes.
There was a problem hiding this comment.
Expected, wrap_comprehension_in feature that we plan to implement.
There was a problem hiding this comment.
These should be fixed in #20768.
This is currently stacked on #20777 to remove the panic from introducing the new syntax error tracked in #20774. I also still need to go through the other new deviations to make sure they look reasonable. Summary -- ```shell git clone git@github.com:psf/black.git ../other/black crates/ruff_python_formatter/resources/test/fixtures/import_black_tests.py ../other/black ``` Then ran our tests and accepted the snapshots I had to make a small fix to our tuple normalization logic for `del` statements in the second commit, otherwise the tests were panicking at a changed AST. I think the new implementation is closer to the intention described in the nearby comment anyway, though. The first commit adds the new files, the next three commits make some small fixes to help get the tests running, and then the last commit accepts all of the new snapshots, including the new unsupported syntax error for one f-string example, tracked in #20774. Test Plan -- Newly imported tests
this was causing an issue with a new black test: ```py del ([], name_1, name_2), [(), [], name_4, name_3], name_1[[name_2 for name_1 in name_0]] ``` we were flattening the first argument since it happened to be a tuple instead of what seems to be the intended case mentioned in the comment where the only `del` target is a tuple (which this example also triggers *after* formatting)
ntBre
force-pushed
the
brent/import-black-tests
branch
from
a48d96d to
a362ddb
Compare
MichaReiser
added
testing
| -]( | ||
| - a: str, | ||
| -): | ||
| +](a: str): |
There was a problem hiding this comment.
Huh, we ignore the trailing comma
2 participants
Summary
Then ran our tests and accepted the snapshots
I had to make a small fix to our tuple normalization logic for
delstatementsin the second commit, otherwise the tests were panicking at a changed AST. I
think the new implementation is closer to the intention described in the nearby
comment anyway, though.
The first commit adds the new Python, settings, and
.expectfiles, the next three commits make some smallfixes to help get the tests running, and then the fifth commit accepts all but one of the new snapshots. The last commit includes the new unsupported syntax error for one f-string example, tracked in #20774.
Test Plan
Newly imported tests. I went through all of the new snapshots and added review comments below. I think they're all expected, except a few cases I wasn't 100% sure about.