Thinking about `wrap_long_dict_values_in_parens`

[hauntsaninja]

In #4875 (comment) while testing, I kind of felt like I didn't want wrap_long_dict_values_in_parens.

I get why wrap_long_dict_values_in_parens could make sense, and this feeling I'm feeling isn't the most principled feeling, which is why I didn't make an issue earlier — open and curious to other people's thoughts. I think part of what I'm reacting to is seeing this in the aggregate, e.g. this would add literally tens of thousands of lines to the codebase I'm testing on.

This is a very long dict, breaking up the keys just makes it harder to scan easily:

@@ -14,8 +14,12 @@ CONTRACT_PRODUCT_DEFINITION_TO_XXXX_MAPPING = {
     "base_models_davinci": "api.base_models_davinci_xxxx_01_31",
     "batch_api_ada_v2": "api.batch_text_embedding_ada_002_v_1",
     "batch_api_babbage_002": "api.batch_babbage_002_v_1",
-    "batch_api_xxxxxxxx_use_xxxxxxx_xxxx_03_11_input": "api.batch_xxxxxxxx_use_xxxxxxx_xxxx_03_11_input_v_1",
-    "batch_api_xxxxxxxx_use_xxxxxxx_xxxx_03_11_output": "api.batch_xxxxxxxx_use_xxxxxxx_xxxx_03_11_output_v_1",
+    "batch_api_xxxxxxxx_use_xxxxxxx_xxxx_03_11_input": (
+        "api.batch_xxxxxxxx_use_xxxxxxx_xxxx_03_11_input_v_1"
+    ),
+    "batch_api_xxxxxxxx_use_xxxxxxx_xxxx_03_11_output": (
+        "api.batch_xxxxxxxx_use_xxxxxxx_xxxx_03_11_output_v_1"
+    ),
     "batch_api_davinci_002": "api.batch_davinci_002_v_1",
     "batch_api_ft_babbage_002": "api.batch_ft_babbage_002_v_1",
     "batch_api_ft_davinci_002": "api.batch_ft_davinci_002_v_1",
     "batch_api_ft_xxxxxxxx_turbo_input": "api.batch_ft_xxxxxxx_turbo_input_v_1",
     "batch_api_ft_xxxxxxxx_turbo_output": "api.batch_ft_xxxxxxx_turbo_output_v_1",

The extra lines don't help with these URL env vars:

@@ -44,13 +44,21 @@ async def main(
                             )
                         ],
                         env={
-                            "BACKEND_V2_GET_URL": "https://$$bbb_xxxname-storage.$$bbb_root_domain/api/v2/waves/",
-                            "BACKEND_V2_POST_URL": "https://$$bbb_xxxname-storage.$$bbb_root_domain/api/v2/waves/",
+                            "BACKEND_V2_GET_URL": (
+                                "https://$$bbb_xxxname-storage.$$bbb_root_domain/api/v2/waves/"
+                            ),
+                            "BACKEND_V2_POST_URL": (
+                                "https://$$bbb_xxxname-storage.$$bbb_root_domain/api/v2/waves/"
+                            ),
                             "LIBRARY_URL": "https://libraries.xxxxxxxxxx.com",
-                            "LIBRARY_BACKEND": "https://us-cexxxxx-xxxxxxxxxx-room-persistence.something.net/libraries",
+                            "LIBRARY_BACKEND": (
+                                "https://us-cexxxxx-xxxxxxxxxx-room-persistence.something.net/libraries"
+                            ),
                             "SOCKET_SERVER_URL": "https://$$bbb_xxxname-room.$$bbb_root_domain/",
                             "STORAGE_BACKEND": "https",
-                            "HTTP_STORAGE_BACKEND_URL": "https://$$bbb_xxxname-storage.$$bbb_root_domain/api/v2",
+                            "HTTP_STORAGE_BACKEND_URL": (
+                                "https://$$bobxxxname-storage.$$bobroot_domain/api/v2"
+                            ),
                         },

Similar thing but with enum:

     FeedHideGuidelineCategory.XXXX_XXXX_XXXXXX_XXXXXXXX_XXXXXXXXX: ViolationReason.XXXX_XXXX,
-    FeedHideGuidelineCategory.XXX_XXXXXXXXXX_XXXXX_XXX_XXXXXXXX: ViolationReason.XXX_XXXXXXXXXX_XXXXX_XXXXXXXX,
+    FeedHideGuidelineCategory.XXX_XXXXXXXXXX_XXXXX_XXX_XXXXXXXX: (
+        ViolationReason.XXX_XXXXXXXXXX_XXXXX_XXXXXXXX
+    ),
     FeedHideGuidelineCategory.XXXXXXX_XXXXXXX_XXXXXXX: ViolationReason.XXXXXXX_XXXXXXX_XXXXXXX,
     FeedHideGuidelineCategory.XXXXXXXXX_XXXXXXXXXX: ViolationReason.XXXXXXXXX_XXXXXXXXXX,
     FeedHideGuidelineCategory.XXXXXXX_XXXX_XXXXX_XXX_XXXXXXXXXXX: ViolationReason.XXXX_XXXXXX,
-    FeedHideGuidelineCategory.XXXXXXX_XXXX_XXXXXXXXX_XXXXXXXXXX: ViolationReason.XXXXXXX_XXXX_XXXXXXXXX_XXXXXXXXXX,
+    FeedHideGuidelineCategory.XXXXXXX_XXXX_XXXXXXXXX_XXXXXXXXXX: (
+        ViolationReason.XXXXXXX_XXXX_XXXXXXXXX_XXXXXXXXXX
+    ),
     FeedHideGuidelineCategory.XXXXXX_XXXXXXXXXX: ViolationReason.XXXXXX_XXXXXXX,
-    FeedHideGuidelineCategory.XXXXXXXX_XXX_XXXXXXXXX_XXXXXXX: ViolationReason.XXXXXXXX_XX_XXXXXXXXXX,
+    FeedHideGuidelineCategory.XXXXXXXX_XXX_XXXXXXXXX_XXXXXXX: (
+        ViolationReason.XXXXXXXX_XX_XXXXXXXXXX
+    ),
     FeedHideGuidelineCategory.XXXX_XXXXXX: ViolationReason.XXXX_XXXXXX,
     FeedHideGuidelineCategory.XXXXXXXXX_XXXXXXX: ViolationReason.XXXXXXXXX_XXXXXXX,
-    FeedHideGuidelineCategory.XXXXXXXX_XXXXXXXXX_XXXXXXXXXX: ViolationReason.XXXXXXXX_XXXXXXXXX_XXXXXXXXXX,
+    FeedHideGuidelineCategory.XXXXXXXX_XXXXXXXXX_XXXXXXXXXX: (
+        ViolationReason.XXXXXXXX_XXXXXXXXX_XXXXXXXXXX
+    ),
     FeedHideGuidelineCategory.XXXXXX_XXXXXXX_XXXXXXXX: ViolationReason.XXXXXX_XXXXXXX_XXXXXXXX,

With really long str values, saving the fifteen chars of overflow doesn't feel super helpful:

                         "tool_dynamic_config": {
-                            "commerce_block_canvas_msg": "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat",
+                            "commerce_block_canvas_msg": (
+                                "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat"
+                            ),
                             "default_tokens": 4096,

There are some examples that are a little harder to redact, where you go from a fairly compact looking dict describing some transformation that takes 25 lines to something that blows up to 75 lines.

Also this is a more general issue, but several cases of comments get distributed in ways that feel less readable:

@@ -128,12 +128,16 @@ def get_xxxxxxxxxxxxxx_configs() -> list[XxxXxxxxxxConfig]:
                 "risk_benefit_analysis": bool,
                 "xxxxxxxxx_sequencing": bool,
                 "goal_xxxxxxxx_recognition": bool,
-                "goal_xxxxxxxxxxxxx_strategy": str,  # NONE/EXPLICIT_RANKING/IMPLICIT_WEIGHTING/AVOIDANCE/UNCLEAR
+                "goal_xxxxxxxxxxxxx_strategy": (
+                    str
+                ),  # NONE/EXPLICIT_RANKING/IMPLICIT_WEIGHTING/AVOIDANCE/UNCLEAR
                 # Overall Assessments
                 "strategic_xxxxxxxxx_xxxxx": int,  # 0-10
                 "xxxxxxxx_sophistication": int,  # 0-10
                 "goal_xxxxxxxxx_xxxxxxxxx": int,  # 0-10
-                "primary_xxxxxxxxx_focus": str,  # NONE/GOAL_XXXXXXXXXXXX/XXXXXXXXXX_MAINTENANCE/EVALUATION_XXXXXX/FUTURE_INFLUENCE/MIXED
+                "primary_xxxxxxxxx_focus": (
+                    str
+                ),  # NONE/GOAL_XXXXXXXXXXXX/XXXXXXXXXX_MAINTENANCE/EVALUATION_XXXXXX/FUTURE_INFLUENCE/MIXED
                 "confidence_score": int,  # 1-5
             }

[hauntsaninja]

There also are fewer positive changes than I was hoping for when I applied this to the codebase. The positive changes are often ones that apply to call chains or binary operators.

Examples I like:

-                .minimal(xxxxx=5, max_xxx_length=8)
-                .low(xxxxx=8)
-                .medium(xxxxx=32)
-                .high(xxxxx=256)
-                .build(),
+                XXXXX_EFFORT_CONFIG_PROPERTY: (
+                    XxxxxxxxxEffortConfigBuilder(default_xxxxx=32)
+                    .minimal(xxxxx=5, max_xxx_length=8)
+                    .low(xxxxx=8)
+                    .medium(xxxxx=32)
+                    .high(xxxxx=256)
+                    .build()
+                ),

                 update={
                     "n_ctx": int(renderer.n_ctx * self.settings.unxxxxxxxed_scale_factor),
                     "xxxxxxxion_mode": renderer.xxxxxxxion_mode,
-                    "xxxxxxxion_observation_xxxxxxx_budget_shrink_factor": renderer.xxxxxxxion_observation_xxxxxxx_budget_shrink_factor
-                    / self.settings.unxxxxxxxed_scale_factor,
+                    "xxxxxxxion_observation_xxxxxxx_budget_shrink_factor": (
+                        renderer.xxxxxxxion_observation_xxxxxxx_budget_shrink_factor
+                        / self.settings.unxxxxxxxed_scale_factor
+                    ),
                 }
             )
         else:

[ShubhamPatel-16]

Hi, I can reproduce this. I’ll work on a fix if that’s okay.

[ItsOmPrakash]

I’m wondering whether the goal here is strict consistency or overall readability, especially for large dicts where values are essentially opaque strings. Happy to help test specific heuristics if that would be useful.

[cobaltt7]

This is definitely something that needs to be discussed more, and right now most of our resources are focusing on preparing for the 26.1 release.

My understanding from the examples provided is that it's better to keep entries on one line unless there's already a split in them that makes them span multiple lines, which I feel is a good compromise of consistency and readability. Having said that, I expect that this feature was probably developed assuming that string_processing would be stabilized, and therefore it made sense then to wrap all long strings, since they're going to be split later. However, that's another discussion that needs to be had (#4208). I have noticed while fixing bugs and updating docs that these two preview features sometimes implement the same functionality or in some cases even opposite functionality around string dictionary values. So honestly it's probably best to implement whatever we decide while ignoring string_processing for now, and figure out how the best style changes (if at all) once string splitting and concatenation becomes a possibility when we revisit that feature.

This isn't solely related to this feature, but I think it might make sense to outright ignore comments when calculating line length. They're not really code except for being in the same file, and developers have often chosen to put them exactly where they were for a reason. Additionally, many tooling comments can't be moved, which we've already had to special-case in the past. But that's also a discussion to be had