Run second pass of formatter #2549
Conversation
|
We already run the formatter twice to address stability issues with the magic trailing comma, so this might be too much. |
| @@ -47,6 +47,9 @@ def _assert_format_equal(expected: str, actual: str) -> None: | |||
| except Exception as ve: | |||
| err(str(ve)) | |||
|
|
|||
| if actual != expected: | |||
| out(diff(expected, actual, "expected", "actual")) | |||
There was a problem hiding this comment.
I found this incredibly helpful in debugging the test failures.
It already prints out the ASTs, but for some of the test files, the ASTs are huge. This prints a diff of the actual output as well.
|
No, we don't (as far as I can tell). That's what this diff addresses. |
|
Sorry. Let me elaborate. I found that we (previously) ran the formatter twice in the validation phase, but not in the actual runtime. Thus the tests passed (since the formatter ran twice), but the actual black executable still had the stability issues. This diff moves the double-execution to the runtime, rather than in the check. |
|
TBH, I'm not sure I fully understand what the fix here is, and the change to consumers. Would black still take the same amount of time to run? |
|
Left some clarifying comments inline in the diff to help clarify what has changed. Difference to consumers is that it's actually stable w.r.t. magic trailing comma. See bug #2518. Previously consumers would have to manually run black twice to work around the bug and achieve correctness. |
| @@ -914,13 +912,17 @@ def format_file_contents(src_contents: str, *, fast: bool, mode: Mode) -> FileCo | |||
| if mode.is_ipynb: | |||
| dst_contents = format_ipynb_string(src_contents, fast=fast, mode=mode) | |||
| else: | |||
| dst_contents = format_str(src_contents, mode=mode) | |||
| dst_contents_pass1 = format_str(src_contents, mode=mode) | |||
| dst_contents = format_str(dst_contents_pass1, mode=mode) | |||
There was a problem hiding this comment.
This actually runs the second pass of the formatter and returns the results of the second pass out
| # forced trailing commas on pass 2) interacting differently with optional | ||
| # parentheses. Admittedly ugly. | ||
| dst_contents_pass2 = format_str(dst_contents, mode=mode) |
There was a problem hiding this comment.
Previously, the second pass only happened here in the check_stability_and_equivalence method, and thus wasn't actually being written out / returned from format_file_contents.
nipunn1313
force-pushed
the
second_pass
branch
from
034a623
to
eb5a4e9
Compare
Fixes psf#2488 and psf#2518 Admittedly, it doesn't fix the underlying issue. There are three expected-failure tests in test_black.py, which prove the issue (test_trailing_comma_optional_parens_stability{1,2,3}) This is a (somewhat terrible) bandaid which runs the entire formatter twice to work around this problem. The problem appears preexisting based on the disabled tests and the comments, so this feels like a reasonable step-in-the-right-direction.
nipunn1313
force-pushed
the
second_pass
branch
from
eb5a4e9
to
2394664
Compare
|
I tend to agree with Jelle. I don't care for bandaid solutions unless really necessary. Having a second pass on validation is good user experience, but this seems really icky since running it twice could potentially obscure even more bugs. Haven't reviewed the code thoroughly yet though, so I might have misunderstood something. |
|
I'm with all of you. I too don't care for bandaid solutions. Currently, there's a bandaid in the validation step (running a second pass before checking stability) which is currently obscuring real stability bugs (#2518). The testsuite would have caught this bug without the validation bandaid. As long as we're running two passes before checking stability in the testsuite, we will continue to obscure true stability bugs - and it's likely further stability regressions will be introduced. Based on this discussion, I'd be in favor of an alternate proposal
I did spend several hours actually trying to actually fix the stability issue, but it was rather difficult to figure out exactly how to solve it. The first pass would omit parens while adding a trailing comma. The second pass would refuse to omit parens because of the trailing comma. Granted I am inexperienced w/ the codebase - maybe someone else will have more success. |
|
I took another go at it and actually was able to come up with a possible solution to the underlying problem in #2572! Guess I had to sleep on it for a week. |
|
Thanks for this, and sorry for not giving it the attention it deserved! I pushed a separate change to run formatting twice, but am now leaning towards removing it again with your #2572 change. |
Description
Run the formatter twice to ensure stability.
Fixes #2488 and #2518
Admittedly, it doesn't fix the underlying issue. There are three
expected-failure tests in test_black.py, which prove the issue
(test_trailing_comma_optional_parens_stability{1,2,3})
This is a (somewhat terrible) bandaid which runs the entire formatter
twice to work around this problem. This bandaid actually already existed
in the stability-checking-validation code, but resulted in a non-stable
black executable. The problem appears preexisting and known
based on the disabled tests and the comments, so this feels like a
reasonable step-in-the-right-direction.
Follow up tasks may want to consider removing this hack and actually
fixing the underlying bug that forces us to run the formatter twice. I tried
to dig into it, but it was pretty tough to figure out. There are some notes in
#2488. The result of
can_omit_invisible_parenschanges during pass1, causingpass2 to do new work that wasn't considered in pass1.
Checklist - did you ...