Avoid extra parentheses for long match patterns with as captures
#21176
Conversation
|
ntBre
added
formatter
😆 |
|
To save you future pain, or rather to bring future pain into the present, I recommend adding some fixtures with absurd comment placement 😄 |
Summary -- This PR fixes #17796 by taking the approach mentioned in #17796 (comment) of simply recursing into the `MatchAs` patterns when checking if we need parentheses. This allows us to reuse the parentheses in the inner pattern before also breaking the `MatchAs` pattern itself. I haven't really resolved the question of whether or not it's okay always to recurse, but I'm hoping the ecosystem check on this PR might shed some light on that. Test Plan -- New tests based on the issue and then reviewing the ecosystem check here
ntBre
force-pushed
the
brent/format-match-as-captures
branch
from
224e099 to
37d447a
Compare
Thanks for the suggestion! I added some more test cases, but I'm definitely open to additional suggestions. I'm not sure I captured the full absurdity available here. I guess I'll tentatively call it okay always to recurse since nothing showed up in the ecosystem check. From what I remember with the syntax errors, many projects still support 3.9 and don't use much pattern matching in general, so I guess this isn't too surprising. The main case I though to worry about was this test case: match class_pattern:
case Class(
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture:
passbut it both wraps the Open to suggestions there as well! |
* main: (188 commits) [ty] Discover site-packages from the environment that ty is installed in (astral-sh#21286) [ty] Make special cases for `UnionType` slightly narrower (astral-sh#21276) Require ignore 0.4.24 in `Cargo.toml` (astral-sh#21292) [ty] Favour imported symbols over builtin symbols (astral-sh#21285) docs: revise Ruff setup instructions for Zed editor (astral-sh#20935) [ty] Update salsa (astral-sh#21281) [syntax-error]: no binding for nonlocal PLE0117 as a semantic syntax error (astral-sh#21032) [ty] Constraining a typevar with itself (possibly via union or intersection) (astral-sh#21273) [`ruff`] Fix false positives on starred arguments (`RUF057`) (astral-sh#21256) [ty] Simplify unions containing multiple type variables during inference (astral-sh#21275) [ty] Add `ty_server::Db` trait (astral-sh#21241) [ty] Refactor `Range` to/from `TextRange` conversion as prep for notebook support (astral-sh#21230) [ty] Fix playground crash when file name includes path separator (astral-sh#21151) [`refurb`] Fix false negative for underscores before sign in `Decimal` constructor (`FURB157`) (astral-sh#21190) [ty] Allow values of type `None` in type expressions (astral-sh#21263) Run codspeed benchmarks with `profiling` profile (astral-sh#21261) [ty] Update expected diagnostic count in benchmarks (astral-sh#21269) Avoid extra parentheses for long `match` patterns with `as` captures (astral-sh#21176) [ty] Update salsa (astral-sh#21265) [ty] `dict` is not assignable to `TypedDict` (astral-sh#21238) ...
Summary
This PR fixes #17796 by taking the approach mentioned in #17796 (comment) of simply recursing into the
MatchAspatterns when checking if we need parentheses. This allows us to reuse the parentheses in the inner pattern before also breaking theMatchAspattern itself:match class_pattern: case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: pass - case ( - Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture - ): + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: pass - case ( - Class( - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - ) as capture - ): + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: pass case ( Class( @@ -685,13 +683,11 @@ match sequence_pattern_brackets: case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: pass - case ( - [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture - ): + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: pass - case ( - [ - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - ] as capture - ): + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: passI haven't really resolved the question of whether or not it's okay always to recurse, but I'm hoping the ecosystem check on this PR might shed some light on that.
Test Plan
New tests based on the issue and then reviewing the ecosystem check here