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

Question about first line of multi-line chain #151

Open
camsteffen opened this issue May 27, 2020 · 7 comments
Open

Question about first line of multi-line chain #151

camsteffen opened this issue May 27, 2020 · 7 comments

Comments

@camsteffen
Copy link

camsteffen commented May 27, 2020

I have a question about Chains of fields and method calls.

We have this statement

If formatting on multiple lines, each field access or method call in the chain should be on its own line...

...and then two exceptions to that statement (if I understand correctly). The first is for cases where the first element is "too short" and causes the second element to float. Makes sense.

The second exception is the following:

Multi-line elements

If any element in a chain is formatted across multiple lines, then that element and any later elements must be on their own line. Earlier elements may be kept on a single line. E.g.,

a.b.c()?.d
    .foo(
        an_expr,
        another_expr,
    )
    .bar
    .baz

This seems like an arbitrary exception to the first statement. Why is the first line allowed to have multiple elements and not broken at .d? Or, why not allow the first line of a chain to contain multiple elements for any chain? Perhaps more elements on the first line should be allowed, but not preferred, for any chain.

I was not able to find rationale for this decision in #66. Sorry if I missed it.

@joshtriplett
Copy link
Member

(I didn't see this issue when it was first filed.)

One justification is that the primary complexity there isn't in the chain length, it's in the multi-line expression. This applies particularly when the multi-line expression contains something like |closure| { ... } or async { ... }.

That said, I do think we should improve our formatting of multi-line chains. For instance, I think it would often be nice if a multi-line expression whose end is just ) or }) or some other series of closing delimiters could have simple one-line chains appended, such as .await? or .context(...)?.

@camsteffen
Copy link
Author

One justification is that the primary complexity there isn't in the chain length, it's in the multi-line expression.

Yeah that makes sense. So should rustfmt follow this rule? It doesn't seem to.

That said, I do think we should improve our formatting of multi-line chains. For instance, I think it would often be nice if a multi-line expression whose end is just ) or }) or some other series of closing delimiters could have simple one-line chains appended, such as .await? or .context(...)?.

Sounds good. I would think that should only apply if there is only one multi-line element which spans every line of the chain. And there can only be one trailing element and optionally one ?.

// good
foo.bar(|| {
    baz;
}).done()?;

// bad, multiple trailing elements
foo.bar(|| {
    baz;
}).hi.await?;

 // bad, multi-line element is not on the first line
foooo
    .bar(|| {
        baz;
    }).await?;

@calebcartwright
Copy link
Member

so should rustfmt follow this rule? It doesn't seem to.

I'm not quite sure I follow this. Could you elaborate on which codified rule in the Style Guide you're saying rustfmt doesn't seem to follow?

@camsteffen
Copy link
Author

camsteffen commented May 2, 2021

Could you elaborate on which codified rule in the Style Guide you're saying rustfmt doesn't seem to follow?

This part:

Earlier elements may be kept on a single line.

So, I would expect this formatting:

a.b.c()?.d
    .foo(|| {
        expr;
    })
    .bar
    .baz

But instead .d is wrapped to its own line.

@calebcartwright
Copy link
Member

I'd have to go back through everything in detail, but I'd assume .d is wrapped due to this other prescription for chain formatting:

If the length of the last line of the first element plus its indentation is less than or equal to the indentation of the second line (and there is space), then combine the first and second lines, e.g.,

The additional rules around multiline elements (the second exception as you phrased it in the description) do not obviate the other formatting rules. So while earlier elements preceding a multiline element may be kept on a single line, whether or not they all are depends on the various other rules

@camsteffen
Copy link
Author

I think that rule is just to prevent a gap like

a
    .b

so that a.b.c are definitely on the same line.

@calebcartwright
Copy link
Member

I think that rule is just to prevent a gap like

Yes, the rule does prevent that scenario, but the style guide rules don't get applied subjectively; it's true for your snippet as well.

It's a matter of (a) which elements can be put together on the same line without exceeding the "small" width constraint, as well as (b) whether the start of an element would drift farther rightward if it were placed at the end of the previous line as opposed to on its own line, as is the case with your example.

Consider this hypothetical and heavily contrived example (with chain_width overridden with a sufficiently high enough value):

    a.b.cxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
        .d
        .foo(|| {
            expr;
        })
        .bar
        .baz

We don't want to tack .d end of that gnarly opening line of elements just because d precedes the multiline foo element.

Similarly, if you slightly adjust your snippet so that the length+indentation calculations are a bit different you'll get:

    ab.c.d
        .foo(|| {
            expr;
        })
        .bar
        .baz

As such the rules are being followed here, but if you think there's any wording changes that would help clarify please let me know or feel free to submit a PR!

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

No branches or pull requests

3 participants