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

Indent lambda parameters if parameters wrap #8465

Closed
wants to merge 2 commits into from

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Nov 3, 2023

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:

def a():
    return b(
        c,
        d,
        e,
        f=lambda 
            self,
            *args,
            **kwargs
        : aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(*args, **kwargs),
        b=d,
        e=f,
    )

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 space in the parameters formatting when the ParameterParentheses::Never is 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

project similarity index total files changed files
cpython 0.75804 1799 1648
django 0.99984 2772 34
home-assistant 0.99963 10596 143
poetry 0.99925 317 12
transformers 0.99967 2657 322
twine 1.00000 33 0
typeshed 0.99980 3669 18
warehouse 0.99977 654 13
zulip 0.99970 1459 21

Preview

project similarity index total files changed files
cpython 0.75804 1799 1648
django 0.99978 2772 40
home-assistant 0.99957 10596 174
poetry 0.99925 317 12
transformers 0.99965 2657 328
twine 1.00000 33 0
typeshed 0.99980 3669 18
warehouse 0.99968 654 16
zulip 0.99970 1459 21

Here a few examples

index 9aa78a281f..499bc0dfb9 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -366,21 +366,21 @@ class BaseExpression:
         internal_type = field.get_internal_type()
         if internal_type == "FloatField":
             return (
-                lambda value, expression, connection: None
-                if value is None
-                else float(value)
+                lambda value, expression, connection: (
+                    None if value is None else float(value)
+                )
             )
         elif internal_type.endswith("IntegerField"):
             return (
-                lambda value, expression, connection: None
-                if value is None
-                else int(value)
+                lambda value, expression, connection: (
+                    None if value is None else int(value)
+                )
             )
         elif internal_type == "DecimalField":
             return (
-                lambda value, expression, connection: None
-                if value is None
-                else Decimal(value)
+                lambda value, expression, connection: (
+                    None if value is None else Decimal(value)
+                )
             )
         return self._convert_value_noop
 
index 889c1cfe84..8555f08cb3 100644
--- a/django/contrib/gis/db/models/fields.py
+++ b/django/contrib/gis/db/models/fields.py
@@ -48,9 +48,9 @@ def get_srid_info(srid, connection):
     alias, get_srs = (
         (
             connection.alias,
-            lambda srid: SpatialRefSys.objects.using(connection.alias)
-            .get(srid=srid)
-            .srs,
+            lambda srid: (
+                SpatialRefSys.objects.using(connection.alias).get(srid=srid).srs
+            ),
         )
         if SpatialRefSys
         else (None, SpatialReference)


index dc857055b1..b45a8bb10e 100644
--- a/django/contrib/admin/tests.py
+++ b/django/contrib/admin/tests.py
@@ -110,8 +110,9 @@ class AdminSeleniumTestCase(SeleniumTestCase, StaticLiveServerTestCase):
         Block until the  page is ready.
         """
         self.wait_until(
-            lambda driver: driver.execute_script("return document.readyState;")
-            == "complete",
+            lambda driver: (
+                driver.execute_script("return document.readyState;") == "complete"
+            ),
             timeout,
         )
 
@@ -196,8 +197,8 @@ class AdminSeleniumTestCase(SeleniumTestCase, StaticLiveServerTestCase):
             # to be the case.
             with self.disable_implicit_wait():
                 self.wait_until(
-                    lambda driver: not driver.find_elements(
-                        By.CSS_SELECTOR, options_selector
+                    lambda driver: (
+                        not driver.find_elements(By.CSS_SELECTOR, options_selector)
                     )
                 )

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
+            )
         ),
         "Database doesn't support feature(s): %s" % ", ".join(features),
         "skipUnlessDBFeature",
@@ -1481,8 +1483,10 @@ def skipUnlessDBFeature(*features):
 def skipUnlessAnyDBFeature(*features):
     """Skip a test unless a database has any of the named features."""
     return _deferredSkip(
-        lambda: not any(
-            getattr(connection.features, feature, False) for feature in features
+        lambda: (
+            not any(
+                getattr(connection.features, feature, False) for feature in features
+            )
         ),
         "Database doesn't support any of the feature(s): %s" % ", ".join(features),
         "skipUnlessAnyDBFeature",

index b9a7e4cd64..a69ebd31cb 100644
--- a/tests/absolute_url_overrides/tests.py
+++ b/tests/absolute_url_overrides/tests.py
@@ -29,8 +29,9 @@ class AbsoluteUrlOverrideTests(SimpleTestCase):
 
         with self.settings(
             ABSOLUTE_URL_OVERRIDES={
-                "absolute_url_overrides.testb": lambda o: "/overridden-test-b/%s/"
-                % o.pk,
+                "absolute_url_overrides.testb": lambda o: (
+                    "/overridden-test-b/%s/" % o.pk
+                ),
             },
         ):

index 4d85d15065..9aec0a68e8 100644
--- a/tests/contenttypes_tests/test_views.py
+++ b/tests/contenttypes_tests/test_views.py
@@ -151,9 +151,9 @@ class ContentTypesViewsSiteRelTests(TestCase):
         The shortcut view works if a model's ForeignKey to site is None.
         """
         get_model.side_effect = (
-            lambda *args, **kwargs: MockSite
-            if args[0] == "sites.Site"
-            else ModelWithNullFKToSite
+            lambda *args, **kwargs: (
+                MockSite if args[0] == "sites.Site" else ModelWithNullFKToSite
+            )
         )
 
         obj = ModelWithNullFKToSite.objects.create(title="title")
@@ -173,9 +173,9 @@ class ContentTypesViewsSiteRelTests(TestCase):
         found in the m2m relationship.
         """
         get_model.side_effect = (
-            lambda *args, **kwargs: MockSite
-            if args[0] == "sites.Site"
-            else ModelWithM2MToSite
+            lambda *args, **kwargs: (
+                MockSite if args[0] == "sites.Site" else ModelWithM2MToSite
+            )
         )

Most of these seem clear improvements to me.

@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 3, 2023

@MichaReiser MichaReiser force-pushed the indent-lambda-params branch 2 times, most recently from 670fea2 to 01c6438 Compare November 3, 2023 07:39
Copy link
Contributor

github-actions bot commented Nov 3, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check detected format changes. (+4 -4 lines in 1 file in 41 projects)

rotki/rotki (+4 -4 lines across 1 file)

rotkehlchen/data_import/importers/binance.py~L272

 
         for rows_group in rows_grouped_by_fee.values():
             rows_group.sort(
-                key=lambda x: x["Change"]
-                if same_assets
-                else x["Change"] * price_at_timestamp[x["Coin"]],
+                key=lambda x: (
+                    x["Change"] if same_assets else x["Change"] * price_at_timestamp[x["Coin"]]
+                ),
                 reverse=True,
             )  # noqa: E501
 

Formatter (preview)

ℹ️ ecosystem check detected format changes. (+91 -71 lines in 15 files in 41 projects)

RasaHQ/rasa (+4 -4 lines across 1 file)

ruff format --preview

rasa/nlu/featurizers/sparse_featurizer/lexical_syntactic_featurizer.py~L86

         "suffix2": lambda token: token.text[-2:],
         "suffix1": lambda token: token.text[-1:],
         "pos": lambda token: token.data.get(POS_TAG_KEY, None),
-        "pos2": lambda token: token.data.get(POS_TAG_KEY, [])[:2]
-        if POS_TAG_KEY in token.data
-        else None,
+        "pos2": lambda token: (
+            token.data.get(POS_TAG_KEY, [])[:2] if POS_TAG_KEY in token.data else None
+        ),
         "upper": lambda token: token.text.isupper(),
         "digit": lambda token: token.text.isdigit(),
     }

aws/aws-sam-cli (+16 -13 lines across 1 file)

ruff format --preview

samcli/lib/cli_validation/image_repository_validation.py~L72

 
             validators = [
                 Validator(
-                    validation_function=lambda: bool(image_repository)
-                    + bool(image_repositories)
-                    + bool(resolve_image_repos)
-                    > 1,
+                    validation_function=lambda: (
+                        bool(image_repository) + bool(image_repositories) + bool(resolve_image_repos) > 1
+                    ),
                     exception=click.BadOptionUsage(
                         option_name="--image-repositories",
                         ctx=ctx,

samcli/lib/cli_validation/image_repository_validation.py~L84

                     ),
                 ),
                 Validator(
-                    validation_function=lambda: not guided
-                    and not (image_repository or image_repositories or resolve_image_repos)
-                    and required,
+                    validation_function=lambda: (
+                        not guided and not (image_repository or image_repositories or resolve_image_repos) and required
+                    ),
                     exception=click.BadOptionUsage(
                         option_name="--image-repositories",
                         ctx=ctx,

samcli/lib/cli_validation/image_repository_validation.py~L94

                     ),
                 ),
                 Validator(
-                    validation_function=lambda: not guided
-                    and (
-                        image_repositories
-                        and not resolve_image_repos
-                        and not _is_all_image_funcs_provided(template_file, image_repositories, parameters_overrides)
+                    validation_function=lambda: (
+                        not guided
+                        and (
+                            image_repositories
+                            and not resolve_image_repos
+                            and not _is_all_image_funcs_provided(
+                                template_file, image_repositories, parameters_overrides
+                            )
+                        )
                     ),
                     exception=click.BadOptionUsage(
                         option_name="--image-repositories", ctx=ctx, message=image_repos_error_msg

demisto/content (+37 -27 lines across 6 files)

ruff format --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py --preview

Packs/FiltersAndTransformers/Scripts/IfElif/IfElif.py~L74

         if "list_compare" in flags:
 
             def to_deep_search(func):
-                return lambda x, y: (func(x, y) or (any(func(x, i) for i in y) if isinstance(y, list) else False))
+                return lambda x, y: func(x, y) or (any(func(x, i) for i in y) if isinstance(y, list) else False)
 
             self.comparison_operators = {k: to_deep_search(v) for k, v in self.comparison_operators.items()}
 

Packs/FireEyeETP/Integrations/FireEyeETPEventCollector/FireEyeETPEventCollector.py~L315

                 last_run_ids: set[str] = {
                     item.get("id", "")
                     for item in filter(
-                        lambda item: datetime.fromisoformat(demisto.get(item, "attributes.meta.last_modified_on"))
-                        == last_run_time,
+                        lambda item: (
+                            datetime.fromisoformat(demisto.get(item, "attributes.meta.last_modified_on")) == last_run_time
+                        ),
                         events,
                     )
                 }

Packs/FireEyeETP/Integrations/FireEyeETPEventCollector/FireEyeETPEventCollector.py~L329

                 last_run_ids = {
                     item.get("id", "")
                     for item in filter(
-                        lambda item: datetime.fromisoformat(demisto.get(item, "attributes.lastModifiedDateTime"))
-                        == last_run_time,
+                        lambda item: (
+                            datetime.fromisoformat(demisto.get(item, "attributes.lastModifiedDateTime")) == last_run_time
+                        ),
                         events,
                     )
                 }

Packs/GigamonThreatINSIGHT/Integrations/GigamonThreatINSIGHT/GigamonThreatINSIGHT.py~L756

             result = getDetectionsInc(detectionClient, result, args)
 
     # filter out training detections
-    result["detections"] = list(filter(lambda detection: (detection["account_uuid"] != TRAINING_ACC), result["detections"]))
+    result["detections"] = list(filter(lambda detection: detection["account_uuid"] != TRAINING_ACC, result["detections"]))
 
     # Include the rules if they need to be included
     if "include" in args and "rules" in args["include"].split(","):

Packs/MailListener/Integrations/MailListenerV2/MailListenerV2.py~L524

 
     return re.sub(
         r"(?P<lseps>\s*)(?P<begin>-----BEGIN(.*?)-----)(?P<body>.*?)(?P<end>-----END(.*?)-----)(?P<tseps>\s*)",
-        lambda m: m.group("lseps").replace(" ", "\n")
-        + m.group("begin")
-        + m.group("body").replace(" ", "\n")
-        + m.group("end")
-        + m.group("tseps").replace(" ", "\n"),
+        lambda m: (
+            m.group("lseps").replace(" ", "\n")
+            + m.group("begin")
+            + m.group("body").replace(" ", "\n")
+            + m.group("end")
+            + m.group("tseps").replace(" ", "\n")
+        ),
         credentials,
         flags=re.DOTALL,
     )

Packs/Slack/Integrations/Slack/Slack.py~L125

         users = json.loads(integration_context["users"])
         users_filter = list(
             filter(
-                lambda u: u.get("name", "").lower() == user_to_search
-                or u.get("profile", {}).get("email", "").lower() == user_to_search
-                or u.get("real_name", "").lower() == user_to_search,
+                lambda u: (
+                    u.get("name", "").lower() == user_to_search
+                    or u.get("profile", {}).get("email", "").lower() == user_to_search
+                    or u.get("real_name", "").lower() == user_to_search
+                ),
                 users,
             )
         )

Packs/Slack/Integrations/Slack/Slack.py~L141

             cursor = response.get("response_metadata", {}).get("next_cursor")
             users_filter = list(
                 filter(
-                    lambda u: u.get("name", "").lower() == user_to_search
-                    or u.get("profile", {}).get("email", "").lower() == user_to_search
-                    or u.get("real_name", "").lower() == user_to_search,
+                    lambda u: (
+                        u.get("name", "").lower() == user_to_search
+                        or u.get("profile", {}).get("email", "").lower() == user_to_search
+                        or u.get("real_name", "").lower() == user_to_search
+                    ),
                     workspace_users,
                 )
             )

Packs/Slack/Integrations/SlackV3/SlackV3.py~L173

     """
     users_filter = list(
         filter(
-            lambda u: u.get("name", "").lower() == user_to_search
-            or u.get("profile", {}).get("display_name", "").lower() == user_to_search
-            or u.get("profile", {}).get("email", "").lower() == user_to_search
-            or u.get("profile", {}).get("real_name", "").lower() == user_to_search,
+            lambda u: (
+                u.get("name", "").lower() == user_to_search
+                or u.get("profile", {}).get("display_name", "").lower() == user_to_search
+                or u.get("profile", {}).get("email", "").lower() == user_to_search
+                or u.get("profile", {}).get("real_name", "").lower() == user_to_search
+            ),
             users_list,
         )
     )

ibis-project/ibis (+6 -4 lines across 1 file)

ruff format --preview

ibis/backends/base/sql/alchemy/init.py~L979

     return (
         sg.parse_one(element.fullname, into=sg.exp.Table, read=dialect)
         .transform(
-            lambda node: node.__class__(this=node.this, quoted=True)
-            if isinstance(node, sg.exp.Identifier)
-            else node
+            lambda node: (
+                node.__class__(this=node.this, quoted=True)
+                if isinstance(node, sg.exp.Identifier)
+                else node
+            )
         )
         .sql(dialect)
     )

pandas-dev/pandas (+18 -13 lines across 4 files)

ruff format --preview

asv_bench/benchmarks/io/style.py~L66

         self.st = self.df.style.apply(_apply_func, axis=1)
 
     def _style_classes(self):
-        classes = self.df.map(lambda v: ("cls-1" if v > 0 else ""))
+        classes = self.df.map(lambda v: "cls-1" if v > 0 else "")
         classes.index, classes.columns = self.df.index, self.df.columns
         self.st = self.df.style.set_td_classes(classes)
 

pandas/core/array_algos/replace.py~L87

         op = lambda x: operator.eq(x, b)
     else:
         op = np.vectorize(
-            lambda x: bool(re.search(b, x))
-            if isinstance(x, str) and isinstance(b, (str, Pattern))
-            else False
+            lambda x: (
+                bool(re.search(b, x))
+                if isinstance(x, str) and isinstance(b, (str, Pattern))
+                else False
+            )
         )
 
     # GH#32621 use mask to avoid comparing to NAs

pandas/core/dtypes/common.py~L129

     Evaluate if the tipo is a subclass of the klasses
     and not a datetimelike.
     """
-    return lambda tipo: (
-        issubclass(tipo, klasses)
-        and not issubclass(tipo, (np.datetime64, np.timedelta64))
+    return (
+        lambda tipo: (
+            issubclass(tipo, klasses)
+            and not issubclass(tipo, (np.datetime64, np.timedelta64))
+        )
     )
 
 

pandas/core/internals/array_manager.py~L403

             Whether to copy the blocks
         """
         return self._get_data_subset(
-            lambda arr: is_numeric_dtype(arr.dtype)
-            or getattr(arr.dtype, "_is_numeric", False)
+            lambda arr: (
+                is_numeric_dtype(arr.dtype) or getattr(arr.dtype, "_is_numeric", False)
+            )
         )
 
     def copy(self, deep: bool | Literal["all"] | None = True) -> Self:

reflex-dev/reflex (+6 -6 lines across 1 file)

ruff format --preview

reflex/components/forms/multiselect.py~L310

         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]
+                )
             ),
         }
 

rotki/rotki (+4 -4 lines across 1 file)

ruff format --preview

rotkehlchen/data_import/importers/binance.py~L272

 
         for rows_group in rows_grouped_by_fee.values():
             rows_group.sort(
-                key=lambda x: x["Change"]
-                if same_assets
-                else x["Change"] * price_at_timestamp[x["Coin"]],
+                key=lambda x: (
+                    x["Change"] if same_assets else x["Change"] * price_at_timestamp[x["Coin"]]
+                ),
                 reverse=True,
             )  # noqa: E501
 

@MichaReiser MichaReiser added formatter Related to the formatter preview Related to preview mode features labels Nov 3, 2023
@MichaReiser MichaReiser changed the base branch from main to fix-multiline-statement-lambda November 3, 2023 08:24
@@ -289,7 +368,9 @@ a = (

# lambda arguments don't have parentheses, so we never add a magic trailing comma ...
def f(
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb = lambda x: y,
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb = lambda
x
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undecided if this looked better before.

# Leading
lambda x: (
lambda y: (
lambda z: (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems better to me



# Leading
lambda x: (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds parentheses, but I don't mind them

c,
d,
e,
f=lambda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks okay.... Parentheses would make it look so much better

@MichaReiser
Copy link
Member Author

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
+            )
         ),

🤷

Base automatically changed from fix-multiline-statement-lambda to main November 5, 2023 14:35
@MichaReiser
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter undocumented deviation: Formatting of long lambda as keyword argument
1 participant