Indent lambda parameters if parameters wrap#8465
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
670fea2 to
01c6438
Compare
crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap
Outdated
Show resolved
Hide resolved
|
01c6438 to
8342fc3
Compare
8342fc3 to
70968de
Compare
| def f( | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb = lambda x: y, | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb = lambda | ||
| x |
There was a problem hiding this comment.
Undecided if this looked better before.
| # Leading | ||
| lambda x: ( | ||
| lambda y: ( | ||
| lambda z: ( |
There was a problem hiding this comment.
This seems better to me
|
|
||
|
|
||
| # Leading | ||
| lambda x: ( |
There was a problem hiding this comment.
This adds parentheses, but I don't mind them
| c, | ||
| d, | ||
| e, | ||
| f=lambda |
There was a problem hiding this comment.
It looks okay.... Parentheses would make it look so much better
70968de to
a3fa295
Compare
|
The main case that is not necessarily better in my view is index 9855c318be..fd7770eb2e 100644
--- a/django/test/testcases.py
+++ b/django/test/testcases.py
@@ -1470,8 +1470,10 @@ def skipIfDBFeature(*features):
def skipUnlessDBFeature(*features):
"""Skip a test unless a database has all the named features."""
return _deferredSkip(
- lambda: not all(
- getattr(connection.features, feature, False) for feature in features
+ lambda: (
+ not all(
+ getattr(connection.features, feature, False) for feature in features
+ )
),🤷 |
e7fb0ac to
3e218fa
Compare
a3fa295 to
2a2886d
Compare
2a2886d to
1534c23
Compare
1534c23 to
d5a18a6
Compare
|
Parenthesizing the body is probably less controversial. It might be worth avoiding to parenthesize the body if it has parentheses itself. But parenthesizing the body otherwise fits with the formatting that copilot would choose. |
Summary -- This PR changes our formatting of `lambda` expressions to keep the parameters on a single line, at least if there are no comments. This fixes #8179. Black formatting and this PR's formatting: ```py def a(): return b( c, d, e, f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa( *args, **kwargs ), ) ``` Stable Ruff formatting ```py def a(): return b( c, d, e, f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(*args, **kwargs), ) ``` I split this off from #21385 because it seemed like the simpler change and helpful to isolate from the body parenthesization ecosystem and performance changes. However, as Micha pointed out, we need the formatting from #21385 to land first, so this branch is currently stacked on that one. Test Plan -- New formatting on tests from #8465 and #21385
…pands (#21385) ## Summary This PR makes two changes to our formatting of `lambda` expressions: 1. We now parenthesize the body expression if it expands 2. We now try to keep the parameters on a single line The latter of these fixes #8179: Black formatting and this PR's formatting: ```py def a(): return b( c, d, e, f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa( *args, **kwargs ), ) ``` Stable Ruff formatting ```py def a(): return b( c, d, e, f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(*args, **kwargs), ) ``` We don't parenthesize the body expression here because the call to `aaaa...` has its own parentheses, but adding a binary operator shows the new parenthesization: ```diff @@ -3,7 +3,7 @@ c, d, e, - f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa( - *args, **kwargs - ) + 1, + f=lambda self, *args, **kwargs: ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(*args, **kwargs) + 1 + ), ) ``` This is actually a new divergence from Black, which formats this input like this: ```py def a(): return b( c, d, e, f=lambda self, *args, **kwargs: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa( *args, **kwargs ) + 1, ) ``` But I think this is an improvement, unlike the case from #8179. One other, smaller benefit is that because we now add parentheses to lambda bodies, we also remove redundant parentheses: ```diff @pytest.mark.parametrize( "f", [ - lambda x: (x.expanding(min_periods=5).cov(x, pairwise=True)), - lambda x: (x.expanding(min_periods=5).corr(x, pairwise=True)), + lambda x: x.expanding(min_periods=5).cov(x, pairwise=True), + lambda x: x.expanding(min_periods=5).corr(x, pairwise=True), ], ) def test_moment_functions_zero_length_pairwise(f): ``` ## Test Plan New tests taken from #8465 and probably a few more I should grab from the ecosystem results. --------- Co-authored-by: Micha Reiser <micha@reiser.io>
Summary -- This PR fixes the issues revealed in #22744 by adding an additional branch to the lambda body formatting that checks if the body `needs_parentheses` before falling back on the `Parentheses::Never` case. I also updated the `ExprNamed::needs_parentheses` implementation to match the one from #8465. Test Plan -- New test based on the failing cases in #22744. I also checked out #22744 and checked that the tests pass after applying the changes from this PR.
Summary -- This PR fixes the issues revealed in #22744 by adding an additional branch to the lambda body formatting that checks if the body `needs_parentheses` before falling back on the `Parentheses::Never` case. I also updated the `ExprNamed::needs_parentheses` implementation to match the one from #8465. Test Plan -- New test based on the failing cases in #22744. I also checked out #22744 and checked that the tests pass after applying the changes from this PR.

Summary
A non-black compatible approach to #8179
The black-compatible formatting would enforce spaces when formatting parameters with the
ParameterParentheses::Never. However, this doesn't work when using comments (which black doesn't support playground?)This PR implements two changes (we may want to split it in two):
a) It indents the lambda parameters if they don't fit on a single line:
b) Preview style that parenthesizes the lambda body if it expands.
return { **super().get_event_triggers(), EventTriggers.ON_CHANGE: ( - lambda e0: [ - Var.create_safe(f"{e0}.map(e => e.value)", _var_is_local=True) - ] - if self.is_multi.equals(Var.create_safe(True)) - else lambda e0: [e0] + lambda e0: ( + [Var.create_safe(f"{e0}.map(e => e.value)", _var_is_local=True)] + if self.is_multi.equals(Var.create_safe(True)) + else lambda e0: [e0] + ) ), }Alternatives
Favor black compatibility and enforce using
spacein the parameters formatting when theParameterParentheses::Neveris used. Requires finding a comment placement that produces stable results.Unclear how we want to support
( lambda x, # comment y: z )Notes
It's probably worth to split out the body changes. They seem to generally improve readability
Test Plan
The non-preview changes don't seem to change the similarity index (at least after only indenting if there are 2 or more parameters).
The preview changes regress the similarity index across the board
Main
Preview
Here a few examples
Most of these seem clear improvements to me.