Formatter: Comprehension ifs should break if the comprehension breaks

[konstin]

Black breaks the if into its own line if the comprehension breaks

a = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    for f in bbbbbbbbbbbbbbb
    if f not in ccccccccccc
)

while we don't

a = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    for f in bbbbbbbbbbbbbbb if f not in ccccccccccc
)

Conversely, parentheses should not require the target breaking

black:

aaaaaaaaaaaaaaaaaaaaa = (
    o for o in self.registry.values if o.__class__ is not ModelAdmin
)

ours:

aaaaaaaaaaaaaaaaaaaaa = (
    o
    for o in self.registry.values if o.__class__ is not ModelAdmin
)

Fixing this requires changing the grouping strategy for comprehensions and generator expressions.

[AbhinavMir]

Happy to work on this. Where would the source for this be? I tried looking in comprehensions and a few other places, but couldn't find the grouping strat. Thx.

[MichaReiser]

Happy to work on this. Where would the source for this be? I tried looking in comprehensions and a few other places, but couldn't find the grouping strat. Thx.

You're in the right file. The grouping of the ifs and adding the line break before it happens here. It may already be sufficient to just move the line break outside of the group.

ruff/crates/ruff_python_formatter/src/other/comprehension.rs

Lines 83 to 89 in cd41474

	joiner.entry(&group(&format_args!(
	leading_comments(own_line_if_comments),
	text("if"),
	trailing_comments(end_of_line_if_comments),
	Spacer(if_case),
	if_case.format(),
	)));

[konstin]

Additionally, you might need to look at expr_list_comp.rs/expr_dict_comp.rs/expr_set_comp.rs (they follow the same structure so i can fix one and then copy that to the others once one works). I think i tried and moving a single group wasn't enough, but it should still be doable by shuffling group()s in the right way.

[AbhinavMir]

Thanks @konstin will get to that too!
@MichaReiser quick question: how does soft_line_break_or_space() decide between line break or space? I assume by moving the line break outside you meant something like this?

write!(
            f,
            [
                text("for"),
                // ADD in line break here!
                trailing_comments(before_target_comments),
                group(&format_args!(
                    Spacer(target),
                    ExprTupleWithoutParentheses(target),
                    in_spacer,
                    leading_comments(before_in_comments),
                    text("in"),
                    trailing_comments(trailing_in_comments),
                    Spacer(iter),
                    iter.format(),
                )),
            ]
        )?;

[MichaReiser]

soft_line_break_or_space renders a space if its enclosing group fits on a line and it renders a line break if it does not.

A group fits on a line if all its content (including nested groups) can be printed on a line without exceeding the configured line width. The way this is determined is by measuring if the groups content and any content coming after the group up to the first line break (soft or hard doesn't matter) doesn't exceed the line width.

You may also want to take a look at the documentation of the different builder methdos (like soft_line_break_or_space). Each of them comes with examples.

moving the line break outside you meant something like this?

Yeah, that's what I had in mind but it may not work (always hard to tell with formatting)