Implement multiline dictionary and list hugging for preview style#8293
Implement multiline dictionary and list hugging for preview style#8293charliermarsh merged 1 commit intomainfrom
Conversation
4a5276f to
73183f0
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no format changes. |
|
Hmm, does it also apply for maybe parenthesize expressions? What parts are missing after this PR? |
|
I think perhaps in |
73183f0 to
e133ec0
Compare
e133ec0 to
619f6e2
Compare
|
(Added |
MichaReiser
left a comment
There was a problem hiding this comment.
Please update your PR summary with the new similarity index and can we get results on our new ecosystem check?
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/hug.py
Show resolved
Hide resolved
|
|
||
| /// Returns `true` if the expression can hug directly to enclosing parentheses. | ||
| /// | ||
| /// For example, in preview style, given: |
There was a problem hiding this comment.
Should we move the preview check into is_huggable to avoid the repetition?
| if options.magic_trailing_comma().is_respect() | ||
| && commas::has_magic_trailing_comma(TextRange::new(arg.end(), item.end()), options, context) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can we add a test case for magic-trailing-comma=ignore covering this check
Just to be clear, we expect no change in the similarity index -- and I don't know if the ecosystem checks include preview style yet, do they @zanieb? The PR shows no change. |
Is it because the projects included in our similarity index using preview-style don't use the new formatting yet? It seems we need to handle match/case as well And can you add tests for delete: |
poetry is formatted with preview style, for the table at https://github.com/astral-sh/ruff/actions/runs/6691118897#summary-18177750624 |
| ) | ||
|
|
||
| foo( | ||
| **[ |
There was a problem hiding this comment.
nit: Use **{} with dict contents here ("TypeError: <...> argument after ** must be a mapping, not list")
b5f1fa9 to
d9d44c1
Compare
|
Poetry's similarity index went down, but I assume that's because this isn't even out in Black yet, so their preview style won't include this. |
|
@charliermarsh what do you plan to do with this PR? |
|
@MichaReiser - Waiting to see if they decide to stabilize or postpone in psf/black#4042. I am also interested in our stance on preview in general -- Black seems to suggest that preview features won't always graduate to stable and may be reverted, but our preview mode was generally intended to be features in which we have high confidence. |
|
I'm okay with landing this now but would recommend documenting the preview checks with the corresponding black preview-code to make it easier to remove the right preview style checks. There's a risk that this might never be stabilized. But for now, the intention is to ship it, just maybe not as part of 2024. |
Can you expand on this a bit more? (Not disagreeing, just didn't fully understand the request.) |
e6754f4 to
ecaec09
Compare
ac6ff26 to
03b2403
Compare
|
👍 Makes sense, will do. |
|
Heads up that Black now collapses these recursively, i.e.: # Input
foo([([
[
1,
2,
3,
]
])])
# Black
foo([([[
1,
2,
3,
]])]) |
|
|
Looking through the ecosystem changes is interesting. I feel like it's fine until you have something like this )
)
if commented_entity:
- entity_map = CommentedMap(
- [(entity["entity"], entity["value"])]
- )
+ entity_map = CommentedMap([(
+ entity["entity"],
+ entity["value"],
+ )])
entity_map.yaml_add_eol_comment(
commented_entity, entity["entity"]
)
entities.append(entity_map)
else:
entities.append(
- OrderedDict(
- [(entity["entity"], entity["value"])]
- )
+ OrderedDict([(
+ entity["entity"],
+ entity["value"],
+ )])
)
else:
entities.append(I guess maybe I'm not into the recursive hugging, it makes it less clear what's going on. |
Yeah, the recursive hugging took me by surprise. I don't mind hugging the first list, but recursively feels a bit excessive. |
|
What is "the first list"? Like, a list within a list, but no deeper? |
Just the top-level list, similar to what Prettier does (yeah, I'm a prettier fan-boy) |
|
Yeah I agree. Perhaps we should take that feedback over to Black? |
|
Ok, I can back out the recursive cases for now (I only added for lists anyway). |
5dd3e0b to
0ab294a
Compare
|
Trying to understand why Poetry's similarity index changed here. Ohh, it looks like we read the Black options from |
|
@MichaReiser - Let me know if you'd like to review. |
MichaReiser
left a comment
There was a problem hiding this comment.
This seems a clear improvement to me. Let's keep the preview style issue open for now and let's document that we don't yet support Black's recursive collapsing.
|
Thanks @MichaReiser! I left the issue open, will defer to you if you choose to close. |
|
This looks fantastic, thank you! pypa/hatch#1085 |
Summary
This PR implement's Black's new single-argument hugging for lists, sets, and dictionaries under preview style.
For example, this:
Would instead now be formatted as:
A couple notes:
It doesn't say it in the originating PR (psf/black#3964), but I think this also applies to parenthesized expressions? At least, it does in my testing of preview vs. stable, though it's possible that behavior predated the linked PR.
See: #8279.
Test Plan
Before:
After: