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

Fix long case blocks not split into multiple lines #4024

Merged
merged 13 commits into from
Nov 7, 2023

Conversation

rdrll
Copy link
Contributor

@rdrll rdrll commented Nov 4, 2023

Description

Fixes #3850 .

This note has been edited for readability and ease of future reference.

Previously Black doesn't recognize case block as one of the syntax blocks need to be wrapped with invisible parentheses, and hence will also not be splitting the case blocks into multiple lines even when it's too long.

Now with (invisible) parentheses correctly wrapped around case blocks, the lines are able to be splitted into multiple lines when needed, this also allows the trailing comma rules to be enabled for case blocks. Unit test pattern_matching_long.py and pattern_matching_trailing_comma.py has been added to verify these modifications.

At start, a side effect of this update is the altered formatting of case case: scenarios, resulting an additional space before the colon (i.e., case case :). This is due to Black after this update will treat the second case as a distinct block that requires invisible parenthesis.

Now, a special patch to the case case: scenario have been made to preserve consistency of user-visible outputs.

A bit of discussion on the side effect prior to the special patch

Some background

Python 3.10 and later support matching the final case with an undefined pattern (e.g. _) as a default when other cases cannot match, this allow code like all following examples to execute and print the "x" without error:

# E.g.1 the usual way for default case
x = 1
match x:
    case 10:
        print(1)
    case _:
        print("x")
# E.g.2 the unusual way 1
x = 1
match x:
    case 10:
        print(1)
    case xxxxxx:
        print("x")
# E.g.3 the unusual way 2
x = 1
match x:
    case 10:
        print(1)
    case case: # note that here the second "case" is an undefined pattern instead of a Python keyword
        print("x")

The side effect

After enabling parentheses to be wrapped around case blocks (hence also line breaking), the output of this particular corner case in our unit test will be different:

match x:
    case case:
        pass

Previously, case blocks were not considered for wrapping with parentheses, but now Black treats the second occurrence of "case" as an actual case block, so an additional space will be added between this occurrence and the colon. The underlying representation Black used for the second line is: "case (case ()):").

Hence, the output is now:

match x:
    case case :
        pass

On the other hand, while the consecutive case block now facing this additional space, the consecutive match block remains unchanged for unit tests like the one below, since the match block have no need of line breaking and was not using invisible parentheses.

match match:
    case x:
        pass

Summary

I was uncertain if it is preferable to add a special patch for this particular case case: scenario since this would adds additional complexity to the codebase. Even with Python 3.10+ allowing to treat the second "case" as an undefined pattern instead of a keyword, I would doubt developers will intentionally use a Python keyword as a pattern/variable.

Nonetheless, considering that the user-visible output should be left as unchanged as possible, a patch have been made to address this.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? not necessary

Copy link

github-actions bot commented Nov 4, 2023

diff-shades reports zero changes comparing this PR (da296f3) to main (46be1f8).


What is this? | Workflow run | diff-shades documentation

@JelleZijlstra
Copy link
Collaborator

Thanks, I think we need to fix the case case : thing. I would classify the extra space there as a bug. If our parser sees this as nested case blocks, that's an implementation detail that shouldn't leak to user-visible behavior.

CHANGES.md Outdated Show resolved Hide resolved
@rdrll
Copy link
Contributor Author

rdrll commented Nov 5, 2023

Thanks for the feedback, I've implemented a fix for the case case: issue.

Now our parser won't attempt to insert the additional space. The unit tests were also modified (reverted) correspondingly to expect no change for this case.

This additional fix caused flake8 to flag the normalize_invisible_parens() function under linegen.py to be C901 too complex (>10 branching logic). Since this function have been the place where we deal with different special cases, I've added an # noqa: C901 comment to suppress the C901 on this function.

Please let me know if changes are needed.

src/black/linegen.py Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Collaborator

Not sure what's going on with the diff-shades job; it passed before but started failing after I merged in main. Possibly it's hanging, as I don't see useful output.

@rdrll
Copy link
Contributor Author

rdrll commented Nov 7, 2023

Yes, this also occured to me earlier once where the diff-shades job failed after running for 1h+, but then it succeeded upon re-running with no actual code change. Maybe I could make some trivial change and see if it can success this time.

@JelleZijlstra JelleZijlstra merged commit 50ed622 into psf:main Nov 7, 2023
41 checks passed
@rdrll rdrll deleted the long_match_case_fixing branch November 7, 2023 20:17
dosisod added a commit to dosisod/black that referenced this pull request Feb 6, 2024
A follow up to psf#4024 but for `if` guards in `case` statements. I noticed this
when psf#4024 was made stable, and noticed I had some code that had extra parens
around the `if` guard.

There might be some edge cases I am not thinking of, but all tests are passing.
I put these new changes under the `remove_redundant_guard_parens` preview name.
JelleZijlstra pushed a commit that referenced this pull request Feb 7, 2024
A follow up to #4024 but for `if` guards in `case` statements. I noticed this
when #4024 was made stable, and noticed I had some code that had extra parens
around the `if` guard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line too long: Many cases in case block
2 participants