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

fix: Wrong indent in some methods invocation #404

Merged
merged 1 commit into from
May 27, 2020

Conversation

Shaolans
Copy link
Member

What changed with this PR:

Fix #293

Example

// Input
class Indent {
 
  void indetMethod() {
    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
    );
    
    assertThat(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      )
      .isEqualTo();

    assertThat(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      )
      .isEqualTo()
      .anotherInvocation(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      );
 
    myinstanceobject
      .assertThat(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      )
      .isEqualTo();


  }
}
// Output
class Indent {

  void indetMethod() {
    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
    );

    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
    )
      .isEqualTo();

    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
    )
      .isEqualTo()
      .anotherInvocation(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      );

    myinstanceobject
      .assertThat(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      )
      .isEqualTo();
  }
}

Relative issues or prs:

@clementdessoude
Copy link
Contributor

Do you have the previous output ?

ctx.primaryPrefix[0].children.fqnOrRefType[0].children.Dot !==
undefined
) &&
// indent when lambdaExpression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent or dedent ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent, we should not dedent when we have a lambdaExpression.
The results are not good.

Copy link
Member Author

@Shaolans Shaolans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have the previous output ?

The previous output is the content below // input.
I have used it to prettify the new version.

ctx.primaryPrefix[0].children.fqnOrRefType[0].children.Dot !==
undefined
) &&
// indent when lambdaExpression
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent, we should not dedent when we have a lambdaExpression.
The results are not good.

@jhaber
Copy link
Contributor

jhaber commented May 27, 2020

Hmm it looks a little odd for the chained method call to just be floating rather than anchored to the close paren, but I don't have a suggestion on how to format it better

@Shaolans
Copy link
Member Author

Hmm it looks a little odd for the chained method call to just be floating rather than anchored to the close paren, but I don't have a suggestion on how to format it better

I am not sure to understand, what do you mean by floating and anchored ?

@jhaber
Copy link
Contributor

jhaber commented May 27, 2020

Sorry if that was unclear, I'm referring to the chained method in an example like this:

    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
    )
      .isEqualTo();

The .isEqualTo() invocation is just kind of floating rather than anchored/aligned with any nearby elements. You can contrast this with something like:

  someOtherMethod()
      .assertThat(
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
        useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
      )
      .isEqualTo();

Where the isEqualTo() invocation is aligned with the close parenthesis

@Shaolans
Copy link
Member Author

Shaolans commented May 27, 2020

That was my first thought but I was not sure, thanks !
I agree that is kind of odd but to me this is expected.
Maybe instead of adding a new line witth ), we can add it at the end of the last argument like so:

    assertThat(
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
      useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa)
      .isEqualTo();

It might be nicer (or not) to look but I don't know if it requires a lot of work on it.

@jhaber
Copy link
Contributor

jhaber commented May 27, 2020

It seems like that would make it harder to read because isEqualTo() and the method arguments aren't clearly distinguished.

Out of curiosity, I just tried this out in the prettier-js playground and it basically used the same format as this PR:

  assertThat(
    useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
    useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa,
    useraaaaaaaaaaojzapjzpozjapjzpoajzpozaaaaaaaaaaaMapperlaaaaaaaaaaaaaaaaaaaaaaaa
  )
    .isNotNull()
    .isEqualTo();

Since this PR matches the prettier-js behavior and I don't have any better suggestions, I'm on board with it 👍

@Shaolans Shaolans merged commit 30f4a0b into jhipster:master May 27, 2020
@Shaolans Shaolans deleted the fix-293 branch May 28, 2020 07:35
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

Successfully merging this pull request may close these issues.

Wrong indent in some methods invocation
3 participants