ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Formatter --preview add trailing comma

Open hoxbro opened this issue 1 year ago • 7 comments

I have the following code:

self._edits.append(
    {"operation": "update", "id": index, "fields": list(fields), "region_fields": []}
)

Which I run with ruff format --preview and gets

self._edits.append({
    "operation": "update",
    "id": index,
    "fields": list(fields),
    "region_fields": [],
})

I would expect it to behave like this based on the new rule hug_parens_with_braces_and_square_brackets

self._edits.append({
    "operation": "update", "id": index, "fields": list(fields), "region_fields": []
})

Playground example: https://play.ruff.rs/6d3bf559-edf0-47f9-b1fd-1fc3418d7a46

hoxbro avatar Dec 06 '23 20:12 hoxbro

Black seems to leave this code unmodified in preview.

charliermarsh avatar Dec 06 '23 20:12 charliermarsh

Black seems to leave this code unmodified in preview.

Not entirely. Black hugs the parentheses, but formats all entries on a single line

self._edits.append({
    "operation": "update", "id": index, "fields": list(fields), "region_fields": []
})

The fix here should be simple, similar to what we do in FormatArgs: Wrap the content (the entries) in a group. This gives us a two-staged formatting: Try to fit the entire dictionary on a line, try to fit the entries on a single line but break the {, }, and last, split each entry on a single line

--- a/crates/ruff_python_formatter/src/expression/expr_dict.rs	(revision Staged)
+++ b/crates/ruff_python_formatter/src/expression/expr_dict.rs	(date 1701914528224)
@@ -60,7 +60,7 @@
             joiner.finish()
         });
 
-        parenthesized("{", &format_pairs, "}")
+        parenthesized("{", &group(&format_pairs), "}")
             .with_dangling_comments(open_parenthesis_comments)
             .fmt(f)
     }

MichaReiser avatar Dec 07 '23 02:12 MichaReiser

I don't see that behavior in the playground (https://black.vercel.app/?version=main&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADbAJNdAD2IimZxl1OAqkg9-7Mdp6WkAR2uY3jqi33CoVxaYgt_dqJQeRWYh2AUUVPYgfmUe5ec4ZcHkdVoq5sOpB1ReRTlQOrJGhEZF9IQqwqtTN2ew7VbGZJ7Tok5yApL_OQ8pjc2rk02ensMLtudWiG0va-XmA-ONGFqx6jYSP7KVMP-bLXSdrnjVEvSF7jcGNtsCCv5gAAASCWI0OPsLj4AAa8B3AEAAEAyP3uxxGf7AgAAAAAEWVo=):

Screen Shot 2023-12-06 at 9 06 03 PM

Maybe I'm doing something wrong.

charliermarsh avatar Dec 07 '23 02:12 charliermarsh

It seems they changed this between Stable and Main. Stable gives you the formatting I shared. Main leaves it as is. I don't like the change. Makes it rather magic and doesn't save on vertical space.

MichaReiser avatar Dec 07 '23 02:12 MichaReiser

Note: The pattern where the "huggable" fully fits on its own line seems somewhat common in Airflow's codebase https://github.com/apache/airflow/pull/37355

MichaReiser avatar Feb 12 '24 11:02 MichaReiser

Related black issue https://github.com/psf/black/issues/4099

There's the option to format this as:

self._edits.append(
    {"operation": "update", "id": index, "fields": list(fields), "region_fields": []}
)

or

self._edits.append({
    "operation": "update", "id": index, "fields": list(fields), "region_fields": []
})

The former probably requires the use of best_fitting![expression.format(), block_indent(&expression.format()), expression.format()] where it should be sufficient to group the pairs for the second one.

MichaReiser avatar Feb 13 '24 12:02 MichaReiser

I remove this from the stable formatter milestone because we decided not to ship the hugging preview style

MichaReiser avatar Feb 14 '24 16:02 MichaReiser