fix: throws rendering

[jhaber]

This is a modified version of #305 that makes a more minor change. Based on #286 it seems like there's not a clear answer on how formatting should work when methods throw lots of exceptions. However, the current behavior where there's an extra newline is basically a bug, for example:

public void method(
  Arg arg1,
  Arg arg2
)
  throws Exception {}

This PR attempts to fix the bug, while punting on the question of a more complete behavior change.

The tests should give a good idea of the behavior I was going for. Unfortunately, my prettier fu is pretty weak so a few of the tests are failing and I'm having a hard time getting things just right

[jhaber]

Updated test output to match current behavior so the tests pass, here's the diff:
HubSpot@6d41b67

There's still an extra newline but only when the throws clause is really long

[clementdessoude]

We could add an extra indent here, so that the throws + exceptionTypeList is not at the same level of the rest of the body, WDYT ?

[jhaber]

Hmm, would that be the only place in prettier-java where a double-indent is used? There is a similar issue with a really long class declaration, for example:

public class MyReallyLongClassName
  extends SomeOtherReallyLongClass
  implements SomeReallyLongInterface {
  private Field field;

  public Field someMethodWithAReallyLongName()
    throws IOException {
    return field;
  }
}

I wonder if the fix in both cases could be to force a newline if the class signature or throws clause linebreaks? This would look like:

public class MyReallyLongClassName
  extends SomeOtherReallyLongClass
  implements SomeReallyLongInterface {

  private Field field;

  public Field someMethodWithAReallyLongName()
    throws IOException {

    return field;
  }
}

[clementdessoude]

There are other places where we could add some double indents indeed.

It might indeed be better to add an extra blank line, but it is more difficult to implement, IMO, as the formatting of the class body is not in the same node as the inheritance/interface declaration.

[jhaber]

Would it be easier if class bodies always started with an empty line? (basically #271)

I guess that wouldn't help the method case though, since you wouldn't want methods to unconditionally start with an empty line

[jhaber]

Also getting back to the original suggestion, double indent (or continuation indent) is pretty common in Java formatters but doesn't seem to be idiomatic in prettier as far as I can tell (this issue didn't seem to go anywhere prettier/prettier#5865).

If we introduced the idea of continuation indent, I think we'd want to apply it consistently. Off the top of my this would include class signatures:

public class MyReallyLongClassName
  ->extends SomeOtherReallyLongClass
  ->implements SomeReallyLongInterface {

Method signatures:

public void myMethod(
  ->String argOne
  ->String argTwo
  ->String argThree
) {}

Method call chains:

return someCollectionType
  ->.stream()
  ->.filter(x -> x != null)
  ->.collect(Collectors.toList());

And probably a bunch of other statements as well. This is definitely one way to go, but would be a pretty massive formatting change at this point. Might be better to move discussion to an issue if that's under consideration

[JanecekPetr]

(Nooo, not in method call chains - those are exactly the type of code that often becomes confusing because there's a continuation on top, and a chained call below, but they look the same. Make split expressions use the continuation indent, and chained calls the "normal", +1, indent...)

[clementdessoude]

Thank you for this @jhaber! One minor comment and we can merge it :)

[jhaber]

Thanks! There is still still an extraneous newline when the throws clause is really long, for example:
HubSpot@6d41b67

I have a hunch I need to use ifBreak somewhere, but couldn't figure out where

[CLAassistant]

All committers have signed the CLA.

[jhaber]

Any more thoughts on this PR? We've been formatting all of our code with this change since April and haven't noticed any undesirable side effects.

[jhaber]

👋 bump on this PR. We're continuing to apply it on top of each prettier-java release for our internal use, but it is a bit of a maintenance burden (and also everyone else doesn't get to benefit 😄 )

[clementdessoude]

I'll think I merge it next week. I will be on holidays so I'll finally have some time :)

[pascalgrimaud]

@Shaolans @Hawkurane : can one of you have a quick look and merge this plz ?

[clementdessoude]

Sorry it took so long @jhaber. I'm merging this. Thank you a lot !