-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[WIP] Implement the wrap_comprehension_in preview style from Black
#21005
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
Conversation
8e1b159 to
d109f48
Compare
|
| /// } | ||
| /// ] | ||
| /// ``` | ||
| fn needs_nested_parentheses(expr: &Expr) -> bool { |
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.
You probably want has_own_parentheses
|
I think the comment here is relevant for your work ruff/crates/ruff_python_formatter/src/expression/mod.rs Lines 386 to 397 in c9dff5c
I'm also not sure if we should implement this preview style, as @dylwil3 pointed out in #20482 (comment) because it might fall into the same category as #12856 where Black now starts introducing otherwise unnecessary parentheses. Are there alternative formattings that we could use that avoid the need for inserting parentheses? |
|
Reading through the issue, the main concern seems to be that very long attribute chains aren't split. That makes me wonder if the proper fix instead is to split attribute expressions in parenthesized expressions. I suspect this would be a bigger change and we probably want to give attribute chains a very low split priority. Or we could decide to only indent the content rather than adding an extra pair of parentheses? |
Summary -- This PR implements the `wrap_comprehension_in` style added in psf/black#4699. This wraps `in` clauses in comprehensions if they get too long. Using some examples from the upstream issue, this code: ```py [a for graph_path_expression in refined_constraint.condition_as_predicate.variables] [ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ] ``` is currently formatted to: ```py [ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ] [ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ] ``` even if the second line of the comprehension exceeds the configured line length. In preview, black will now break these lines by parenthesizing the expression following `in`: ```py [ a for graph_path_expression in ( refined_constraint.condition_as_predicate.variables ) ] [ a for graph_path_expression in ( refined_constraint.condition_as_predicate.variables ) ] ``` I actually kind of like the alternative formatting mentioned on the original Black issue and in our #12870 which would be more like: ```py [ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ] ``` but I think I'm in the minority there. Test Plan -- Existing Black compatibility tests showing fewer differences
```py [a for graph_path_expression in refined_constraint.condition_as_predicate.variables] ``` ```shell $ cargo run -p ruff -- format --check --preview --no-cache --config "line-length=79" fmt.py unformatted: File would be reformatted --> fmt.py:1:1 - [a for graph_path_expression in refined_constraint.condition_as_predicate.variables] 1 + [ 2 + a 3 + for graph_path_expression in ( 4 + refined_constraint.condition_as_predicate.variables 5 + ) 6 + ] ```
d109f48 to
e18ead1
Compare
…21021) ## Summary I spun this out from #21005 because I thought it might be helpful separately. It just renders a nice `Diagnostic` for syntax errors pointing to the source of the error. This seemed a bit more helpful to me than just the byte offset when working on #21005, and we had most of the code around after #20443 anyway. ## Test Plan This doesn't actually affect any passing tests, but here's an example of the additional output I got when I broke the spacing after the `in` token: ``` error[internal-error]: Expected 'in', found name --> /home/brent/astral/ruff/crates/ruff_python_formatter/resources/test/fixtures/black/cases/cantfit.py:50:79 | 48 | need_more_to_make_the_line_long_enough, 49 | ) 50 | del ([], name_1, name_2), [(), [], name_4, name_3], name_1[[name_2 for name_1 inname_0]] | ^^^^^^^^ 51 | del () | ``` I just appended this to the other existing output for now.
|
Thanks for the pointers here and in our 1:1! The draft is in a slightly better state now, at least in terms of the tests and the ecosystem results. However, I'm now splitting expressions like this too eagerly on the unformatted: File would be reformatted
--> /tmp/tmp.h5wCjfHytw/try.py:1:1
2 | async def api_get_user_extensions(
3 | user: User = Depends(check_user_exists),
4 | ) -> list[Extension]:
-
- user_extensions_ids = [
- ue.extension for ue in await get_user_extensions(user.id)
- ]
- return [
- ext
- for ext in await get_valid_extensions(False)
- if ext.code in user_extensions_ids
- ]
5 + user_extensions_ids = [ue.extension for ue in await get_user_extensions(user.id)]
6 + return [ext for ext in await get_valid_extensions(
7 + False
8 + ) if ext.code in user_extensions_ids]Similar cases make up all of the ecosystem results I've looked at. Is there an easy way to avoid that? It does seem to be something with I will probably move on to another preview style as you and Dylan said, unless this is looking promising. It seems like there are other designs worth exploring in the future anyway. |
This makes me wonder if you even want
|
|
This doesn't feel very formal, so I don't think it fully answers your questions, but my understanding based on the black tests are that this preview style should only come into effect after all of the usual splits have been done and the @extension_router.get("")
async def api_get_user_extensions(
user: User = Depends(check_user_exists),
) -> list[Extension]:
user_extensions_ids = [
ue.extension for ue in await get_user_extensions(user.id)
]
return [
ext
for ext in await get_valid_extensions(False)
if ext.code in user_extensions_ids
]If I extend the @extension_router.get("")
async def api_get_user_extensions(
user: User = Depends(check_user_exists),
) -> list[Extension]:
user_extensions_ids = [
ue.extension for ue in await get_user_extensions(user.id)
]
return [
ext
for ext in await get_valid_extensions(
Falseeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
)
if ext.code in user_extensions_ids
]which also makes sense to me. This part is a bit confusing to me, but if I remove the @extension_router.get("")
async def api_get_user_extensions(
user: User = Depends(check_user_exists),
) -> list[Extension]:
user_extensions_ids = [
ue.extension for ue in await get_user_extensions(user.id)
]
return [
ext
for ext in aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
]If I make that an attribute expression instead (replacing an @extension_router.get("")
async def api_get_user_extensions(
user: User = Depends(check_user_exists),
) -> list[Extension]:
user_extensions_ids = [
ue.extension for ue in await get_user_extensions(user.id)
]
return [
ext
for ext in (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
)
]I get that same splitting even with a following Black also explicitly tests that they don't parenthesize the So at least from these tests, it seems that the comprehension always gets fully expanded like the stable behavior, and then if the for-in part is still too long, and it's a specific type of expression like an attribute chain, it can be parenthesized and split across multiple lines. Does that help at all? Maybe I need to take a look at |
|
Thanks for explaining. What I understand is:
But you changed more than just from identifier to attribute. The attribute expression is shorter. I think this is just
We did implement this behavior in a few places but it's fairly tricky and I'm honestly not sure if it's worth it. Some users find it confusing (it goes against: predictable formatting), but it does avoid parentheses in some places, like assignment, where I think it makes sense. As discussed with Dylan. I'm not convinced that this always improves formatting. Have you considered alternative formattings that don't require inserting parentheses but result in the same improvement? Either way. I haven't seen this come up. I suggest deprioritizing it. We have other formatter issues that have been requested more frequently (I don't remember that this ever came up) |
Sounds good, thanks for talking through it. I still learned a lot!
I kind of liked this formatting mentioned on the upstream issue, but that's about as much as I've considered it. |
This is a rough draft with a naive fix. We still disagree with Black on some formatting, for example this snippet from our docstring formatting tests:
Black reuses the parentheses from the
sortedcall instead of adding new parentheses around the whole thing, which seems preferable.Black:
This PR:
I can't quite tell if I'm having trouble here because this is tricky to implement in Ruff as Dylan mentioned here, or if I'm still just unfamiliar with the formatter.
Summary
This PR implements the
wrap_comprehension_instyle added inpsf/black#4699. This wraps
inclauses incomprehensions if they get too long. Using some examples from the upstream
issue, this code:
is currently formatted to:
[ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ] [ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ]even if the second line of the comprehension exceeds the configured line length.
In preview, black will now break these lines by parenthesizing the expression
following
in:[ a for graph_path_expression in ( refined_constraint.condition_as_predicate.variables ) ] [ a for graph_path_expression in ( refined_constraint.condition_as_predicate.variables ) ]I actually kind of like the alternative formatting mentioned on the original
Black issue and in our #12870 which would be more like:
[ a for graph_path_expression in refined_constraint.condition_as_predicate.variables ]but I think I'm in the minority there.
Test Plan
Existing Black compatibility tests showing fewer differences