Skip to content
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

Formatter: With / AsyncWith (includes WithItem) #5368

Closed
2 of 3 tasks
Tracked by #4798
MichaReiser opened this issue Jun 26, 2023 · 6 comments · Fixed by #5758
Closed
2 of 3 tasks
Tracked by #4798

Formatter: With / AsyncWith (includes WithItem) #5368

MichaReiser opened this issue Jun 26, 2023 · 6 comments · Fixed by #5758
Assignees
Labels
formatter Related to the formatter

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Jun 26, 2023

@MichaReiser
Copy link
Member Author

With has been implemented by #5350

@davidszotten do you want to look into/plan to work on generalizing the logic by either defining an union enum or a trait for AsyncWith?

@MichaReiser MichaReiser added the formatter Related to the formatter label Jun 26, 2023
@davidszotten
Copy link
Contributor

sure, will take a look

@MichaReiser
Copy link
Member Author

I'm not sure how to handle this inside of the formatter but we're currently adding to many parentheses for

# Input
with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) as c:
    ...

# Ruff Output
with (
	(
		aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
	) as c,
):
	...

# Black
with (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
) as c:
    ...

Because both the binary expression and the with statement are adding parentheses. It may be necessary to explicitly enumerate all the expressions that support breaking the left side, similar to can_break_expr OR rethink how we group elements, potentially use best_fitting.

Other interesting patterns

with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) as c, (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) as c:
    ...

with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb), (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb):
    ...

with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c:
    ...

We don't need to match black precisely but I do prefer black's formatting overall.

We can tackle this in a separate PR and I can also look into it

@davidszotten
Copy link
Contributor

davidszotten commented Jun 26, 2023

i was trying to follow the code to understand how if_group_breaks works (but failed). does the outer group also consider itself broken if the inner one does? or what is causing the outer group to expand?

@MichaReiser
Copy link
Member Author

i was trying to follow the code to understand how if_group_breaks works (but failed). does the outer group also consider itself broken if the inner one does? or what is causing the outer group to expand?

That's correct. An outer group always expands if an inner group expands. I'm on my phone but there's a propagate_expands that implements this behavior.

A sequence of groups expands the groups from right to left

Best fitting allows you to provide multiple versions of the same content and the printer tries to pick the first one that fits (or falls back to an expanded version)

If group breaks accepts an optional with_group. It makes the content dependent on whatever the group with that given id expands rather than the enclosing group. However, the referred group must either be a parent or precede the content.

I think that summarizes most grouping IR elements.

In this case, it may be necessary that we enumerate all expressions for which we don't want parantheses.

@davidszotten
Copy link
Contributor

ok. might leave this for you (separate pr seems ok)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants