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: break long assignments after equals #564

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

jtkiesel
Copy link
Contributor

@jtkiesel jtkiesel commented Nov 27, 2022

What changed with this PR:

Long assignments (i.e. those that previously violated printWidth) of many types now break after equals the way Prettier JavaScript now does.

Example

Input

class Example {

  void example() {
    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes = new Object()
      .someMethod();

    Object[] aParticularlyLongAndObnoxiousNameForIllustrativePurposes2 = new Object[] {
      new Object(),
      new Object()
    };

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes3 = SomeClass.someStaticMethod();

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes = someMethod()
      .anotherMethod();

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes = anotherValue !=
      null
      ? anotherValue
      : new Object();
  }
}

Output

class Example {

  void example() {
    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes =
      new Object().someMethod();

    Object[] aParticularlyLongAndObnoxiousNameForIllustrativePurposes2 =
      new Object[] { new Object(), new Object() };

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes3 =
      SomeClass.someStaticMethod();

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes =
      someMethod().anotherMethod();

    Object aParticularlyLongAndObnoxiousNameForIllustrativePurposes =
      anotherValue != null ? anotherValue : new Object();
  }
}

Relative issues or prs:

Fix #527

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2022

CLA assistant check
All committers have signed the CLA.

@jtkiesel
Copy link
Contributor Author

@clementdessoude I don't mean to sound impatient, so I apologize in advanced, I just wanted to make sure that I have done everything I needed to do for this pull request. I'm not sure if there's anything I'm missing, or if anyone else handles pull request reviews/merges. Any chance this could get a (hopefully quick) review?

const groupId = Symbol("assignment");
return group([
group(variableDeclaratorId),
" =",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that with this implementation, some (oddly placed) comment might disapear. For instance, I think that in one of these two statements, the comment will disapear, as it is attached to the ctx.Equals[0] symbol.

private Map<Integer, String> genericVariable4 /* comment */= new HashMap<Integer, String>(); or private Map<Integer, String> genericVariable4 =/* comment */ new HashMap<Integer, String>();

Can you try to add this test case and check if it is the case ? If the comment disappears, you can try to replace " =" by " ", ctx.Equals[0], here and in the three declarations afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I thought there might be something I was missing. I tested the two scenarios you presented, and the first case (comment before equals) caused the comment to disappear, while the second care (comment after equals) didn't. (I assume in the 2nd case, the comment becomes "attached" to the value, rather than the equals.) I just pushed a fix for it.

Comparing the way that comments are printed to Prettier JavaScript, Prettier Java seems to be a bit different (Prettier JavaScript always separates comments from other elements with a single space, while Prettier Java omits the space), but I confirmed that the comment printing behavior isn't changed by this PR (anymore).

@clementdessoude
Copy link
Contributor

Once again, I'm so sorry for the very long delay in the response. Thanks for pinging me back again !

I have a small comment, let me know if you can tackle it. Otherwise, I should be able to test it in the evening or tomorrow.

@jtkiesel
Copy link
Contributor Author

No worries at all, I'm sorry if I came off as impatient or rude. Thank you for taking the time to review!

@clementdessoude
Copy link
Contributor

Well done ! I had just one doubt I wanted to clear up before merging, but everything looks fine ! Thanks a lot @jtkiesel !

@clementdessoude clementdessoude merged commit 3358ada into jhipster:main Mar 4, 2023
@jtkiesel jtkiesel deleted the fix/break-after-equals branch March 4, 2023 15:41
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.

Print width not obeyed on assignments
3 participants