rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

Fix/Visual IndentStyle is broken on impl with where clause

Open jmj0502 opened this issue 2 years ago • 15 comments

Fixes #3071

jmj0502 avatar Jul 21 '22 01:07 jmj0502

@jmj0502 Thank you for your second contribution now! I want to let you know that I've seen this but likely won't get around to reviewing it until later next week.

ytmimi avatar Jul 22 '22 20:07 ytmimi

@ytmimi Thanks for all the guidance! Looking forward to keep on contributing and getting to know the project better, in order to tackle tougher issues in the future. In regards to the review process; no worries, I'll keep an eye around in case changes are required, so don't hesitate to reach out 👍 .

jmj0502 avatar Jul 22 '22 23:07 jmj0502

@ytmimi Hey! Hope you're doing great! c: Will this PR land? 😢

jmj0502 avatar Aug 06 '22 13:08 jmj0502

@jmj0502 Don't worry, I haven't forgotten about this, I just haven't had as much time as I'd like for reviews recently. Apologies for the delay and I'll look to provide you with some feedback on this soon

ytmimi avatar Aug 08 '22 00:08 ytmimi

@jmj0502 thanks for looking into this. I have two asks before moving forward

  1. Could we update the Where predicates section of the indent_style docs in Configurations.md to add a short code snippet that shows the formatting of where predicates in impl block for both the Block and Visual case.
  2. Could we add a test where where_single_line=true. I want to make sure that these changes don't affect that option. I'm actually not sure if that option has any affect on impl blocks, but just want to check the behavior when the option is set to true and indent_style=Visual.

ytmimi avatar Aug 17 '22 13:08 ytmimi

@ytmimi Cool! I'll add the changes as soon as I have some free time ✌️

jmj0502 avatar Aug 17 '22 13:08 jmj0502

@ytmimi Done! I added the additional tests and some code snippets that illustrate how the format of the where clauses differ based on the value provided to indent_style.

jmj0502 avatar Aug 17 '22 18:08 jmj0502

@ytmimi I'm getting format errors each time I run the tests for Configurations.md file. I think I missed something. Any clues?

jmj0502 avatar Aug 17 '22 21:08 jmj0502

hmmm 🤔 not sure either, but I'm looking into it!

ytmimi avatar Aug 18 '22 12:08 ytmimi

It looks like these are the snippets rustfmt accepts. I took each one and ran rustfmt on it with the indent_style config. What I find odd is that both struct cases want to keep the opening brace on the same line as the bounds, which seems inconsistent but not something we should tackle right now. Personally I'd recommend removing that example right now

For your info Each code snippet should represent what the code would look like after rustfmt has been applied to it. So basically the snippets need to be idempotent.

Here's the Block formatting snippet

fn lorem<Ipsum, Dolor, Sit, Amet>() -> T
where
    Ipsum: Eq,
    Dolor: Eq,
    Sit: Eq,
    Amet: Eq,
{
    // body
}

struct Foo<T>
where
    T: Eq, {
    // body
}

impl<T> Foo<T>
where
    T: Eq,
{
    // body
}

Here's the Visual formatting snippet.

fn lorem<Ipsum, Dolor, Sit, Amet>() -> T
    where Ipsum: Eq,
          Dolor: Eq,
          Sit: Eq,
          Amet: Eq
{
    // body
}

struct Foo<T>
    where T: Eq {
    // body
}

impl<T> Foo<T>
    where T: Eq
{
    // body
}

ytmimi avatar Aug 18 '22 13:08 ytmimi

@jmj0502 I opened up a new issue for this odd struct behavior in #5507

ytmimi avatar Aug 18 '22 13:08 ytmimi

@ytmimi Okok, I'll proceed to remove the examples 👍

jmj0502 avatar Aug 18 '22 13:08 jmj0502

@ytmimi Okok, I'll proceed to remove the examples 👍

To be clear, I'd like to keep the impl example since that's what this PR focused on, but we can remove the struct example for now until we sort out what's going on in the odd case of an empty body + where clause.

ytmimi avatar Aug 18 '22 13:08 ytmimi

@ytmimi Okok, no worries! Looking forward to eventually re-introduce such examples (possibly in the PR that solves #5507).

jmj0502 avatar Aug 18 '22 14:08 jmj0502

@ytmimi Donee!

jmj0502 avatar Aug 18 '22 15:08 jmj0502