-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement multiline dictionary and list hugging for preview style #8293
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -1007,6 +1025,75 @@ pub(crate) fn has_own_parentheses( | |||
} | |||
} | |||
|
|||
/// Returns `true` if the expression can hug directly to enclosing parentheses. | |||
/// | |||
/// For example, in preview style, given: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the preview check into is_huggable
to avoid the repetition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: