Inconsistent wrapping for long call chain

[PeterJCLaw]

Method chaining is inconsistent when the overall expression is a call (rather than, say, an assignment). Not sure if this is a style change or a bug.

Describe the style change

It would be great if black were consistent in its wrapping such that it either chained horizontally or vertically, but not both within the same expression (at least within the same level in the call-nesting of an expression).

Examples in the current Black style

def func():
    thing("argument-1").thing("argument-2").thing("argument-3").thing(
        "argument-4",
    ).thing("argument-5").thing("argument-6").thing("argument-7").thing(
        "argument-8",
    ).thing(
        "argument-9",
    ).thing(
        "argument-10"
    ).thing(
        "argument-11"
    ).thing(
        "argument-12"
    )

Adding trailing commas doesn't help, though I understand that that's a separate issue (#1671):

def func():
    thing("argument-1",).thing("argument-2",).thing("argument-3",).thing(
        "argument-4",
    ).thing("argument-5",).thing("argument-6",).thing("argument-7",).thing(
        "argument-8",
    ).thing(
        "argument-9",
    ).thing(
        "argument-10"
    ).thing(
        "argument-11"
    ).thing(
        "argument-12"
    )

Desired style

def func():
    thing(
        "argument-1",
    ).thing(
        "argument-2",
    ).thing(
        "argument-3",
    ).thing(
        "argument-4",
    ).thing(
        "argument-5",
    ).thing(
        "argument-6",
    ).thing(
        "argument-7",
    ).thing(
        "argument-8",
    ).thing(
        "argument-9",
    ).thing(
        "argument-10"
    ).thing(
        "argument-11"
    ).thing(
        "argument-12"
    )

Additional context

Possibly related issues:

• Only the last trailing comma in a statement is magic #1671
• Strange formatting of fluent interface #571
• Inconsistent method chaining #1723

[bmustiata]

+1 The current behavior causes problems whenever there's a chained operation of any sort.

It is even worse if there are statements such as:

start(name="task a")\
    .something("...")\
    .start(name="task a.1")\
        .something("...")\
        .something("...")\
    .end()\
.end()    

Having it linear gives us at least a chance to deal with this.

Another option would be to create some black ignored sections?

# black: ignore
start(name="task a")\
    .something("...")\
    .start(name="task a.1")\
        .something("...")\
        .something("...")\
    .end()\
.end()
# black: ignoreend

[JelleZijlstra]

Another option would be to create some black ignored sections?

We have that, it's called # fmt: off.

[felix-hilden]

Actually, in line with the documentation, I'd expect it to be formatted like this:

def func():
    (
        thing("argument-1")
        .thing("argument-2")
        .thing("argument-3")
        .thing("argument-4")
        .thing("argument-5")
        .thing("argument-6")
        .thing("argument-7")
        .thing("argument-8")
        .thing("argument-9")
        .thing("argument-10")
        .thing("argument-11")
        .thing("argument-12")
    )

which Black does keep (playground), but doesn't quite produce.

[felix-hilden]

But because of the breaks in parentheses and lack of optional parens, I'm going to say this is related to #236 and #348 and the like. Assuming my example is the desired one.

[PeterJCLaw]

I agree that #2279 (comment) is better than the current output, however due to the presence of trailing commas here I would expect that this would be wrapped as in my original report.
I'm not sure what black should do given a similar input which didn't have trailing commas.

[JelleZijlstra]

@1oglop1 that's #657 I believe.

[1oglop1]

@JelleZijlstra Thank you for correcting me I moved the comment there. I was not sure how this feature is called, however I got bitten by the call chain as well :)
I'd also prefer #2279(comment)

[peterHoburg]

To add to this. While using SQLAlchemy and long chained ORM query calls black has weird formatting.

Here is what black does

    sql_alchemy_example = session.query(
        model.a,
        model.b,
        func.row_number()
        .over(
            partition_by=[
                model.c,
                model.d,
            ],
            order_by=[
                model.f,
            ],
        )
        .label("row_number"),
    ).where(
        model.j > 1,
    )

Here is what I expect it to do VIA the Black docs:

    sql_alchemy_example = (
        session.query(
            model.a,
            model.b,
            func.row_number()
            .over(
                partition_by=[
                    model.c,
                    model.d,
                ],
                order_by=[
                    model.f,
                ],
            )
            .label("row_number"),
        )
        .where(
            model.j > 1,
        )
    )

[felix-hilden]

I think this is essentially the same as #517, which has been mentioned here a couple of times already. So let's continue the discussion there.

[PeterJCLaw]

@felix-hilden apologies if I'm missing something, but I'm not sure how this relates to #517? That issue looks like it's a release or release-process related issue, while this one describes a particular formatting bug. Is there a typo in the number you're referring to?

[felix-hilden]

Oh, indeed! It should be #571 👍

[PeterJCLaw]

Aha, sorry, yeah, just found that myself. I think that this case is different to that one as this issue relate to statements which are not themselves within a parenthesised block, that is:

x  = (
    foo().bar().etc()
)

vs

x = foo().bar().etc()

While there is some similarity in how Black handles those cases, I believe they are separate cases. (I've not tested this example recently, but at the time of raising I believe Black's behaviour in the parenthesised case for otherwise similar input code was quite different to the issue reported here).

[felix-hilden]

I agree with you there, but whatever fix we apply should definitely take care of both cases at the same time. Whether they are somehow separate in our implementation or not is a different consideration. They seem to be formatted similarly (and correctly), but what doesn't quite work is adding the assignment with the arguments. I'll add this distinction to the other issue as well.

[PeterJCLaw]

Ah, possibly I've confused things with that example? Here's a comparison of black handling the code input from this issue, with and without surrounding parens.

The output is rather different between the two inputs (though I'd argue neither is great in their current form). While I don't entirely disagree that a fix would ideally address both, what complicates this is that the expected behaviour in each case is different. For parenthesised expressions Black tends to wrap at the member-separator, while for non-parenthesised ones it wraps at method arguments ¹.

For clarity: the issue here is that it's not achieving a self-consistent style given the non-parenthesised non-assignment input, rather whether Black should aim for the with- and without-parens (or with/without assignment) cases to behave the same.

Footnotes

1. I have opinions about which of these is nicer (I prefer breaks in arguments, with trailing commas, no extra parens), but that's not the purpose of this issue. ↩

[felix-hilden]

I think I get what you're saying. And I'd expect trailing commas to behave exactly as they do in your last parenthesised example, exploding the individual call parens.

#571 suggests that we don't break inside calls, and you prefer breaks in calls. But because we advertise formatting call chains by separating from dots, I'd say the expected result you provided goes against that to a degree. So let's try to hash it out. I think whether or not the expression has been parenthesised, or if it's an assignment should not affect the result. And even if they should be different, I'd prefer we determine that on a single issue. I'll try to come up with a more comprehensive set of examples.