jhipster / prettier-java

Prettier Java Plugin
http://www.jhipster.tech/prettier-java/
Apache License 2.0
1.06k stars 103 forks source link

Don't split a short line static method to two lines. #626

Closed xenoterracide closed 4 months ago

xenoterracide commented 6 months ago

putting Try and of on separate lines here seems weird.

Input:

    private static <T extends Group> Function<Constructor<?>, T> createInstance(Group entity) {
        return ctor -> Try.of(() -> {
                    @SuppressWarnings("unchecked")
                    var ng = (T) ctor.newInstance(entity.getId(), entity.getSystemGenerated(), entity.getVersionKey());
                    return ng;
                })
                .getOrElseThrow(ex -> new RuntimeException(ex));
    }

Output:

  private static <T extends Group> Function<Constructor<?>, T> createInstance(Group entity) {
    return ctor ->
      Try
        .of(() -> {
          @SuppressWarnings("unchecked")
          var ng = (T) ctor.newInstance(entity.getId(), entity.getSystemGenerated(), entity.getVersionKey());
          return ng;
        })
        .getOrElseThrow(ex -> new RuntimeException(ex));
  }

Expected behavior:

  private static <T extends Group> Function<Constructor<?>, T> createInstance(Group entity) {
    return ctor ->
      Try.of(() -> {
          @SuppressWarnings("unchecked")
          var ng = (T) ctor.newInstance(entity.getId(), entity.getSystemGenerated(), entity.getVersionKey());
          return ng;
        })
        .getOrElseThrow(ex -> new RuntimeException(ex));
  }

or perhaps even this since the return is implicit.

    return ctor -> Try.of(() -> {

Configuration:

  "dependencies": {
    "prettier": "^3.1.1",
    "prettier-plugin-java": "^2.5.0"
  }
printWidth: 120
plugins:
  - prettier-plugin-java
root = true

[*]
trim_trailing_whitespace = true
end_of_line = lf
insert_final_newline = true
charset = utf-8
indent_style = space
indent_size = 2

[*.bat]
end_of_line = crlf
jhaber commented 6 months ago

I think it would be weird to have the .of not line-broken, but have the .getOrElseThrow line-broken later. It seems like prettier and prettier-java both try to follow the philosophy where as soon as one element in an expression get line-broken, then every element gets line-broken (same idea for call chains and method arguments). This avoids any heuristics so you get simple, predictable behavior, it scales up very well to arbitrarily complex nested expressions, and the downside is you end up with a lot of linebreaks

xenoterracide commented 6 months ago

maybe, although, now I find myself trying to recall if typescript has static functions on classes. usually if you want a static function it doesn't go into a class itself. Do what though wilt. I just thought it was odd, but it's not a huge deal. We were comparing style outputs between a few formatters, and I generally use this one ;) .

jtkiesel commented 6 months ago

I believe the TypeScript equivalent of the input you provided would roughly be:

class Example {
    private static createInstance<T extends Group>(entity: Group): (ctor: { newInstance: (...args: unknown[]) => unknown }) => T {
        return ctor => Try.of(() => {
                    var ng = ctor.newInstance(entity.getId(), entity.getSystemGenerated(), entity.getVersionKey()) as T;
                    return ng;
                })
                .getOrElseThrow(ex => new RuntimeException(ex));
    }
}

Such an input yields the following output from Prettier (playground):

class Example {
    private static createInstance<T extends Group>(
        entity: Group,
    ): (ctor: { newInstance: (...args: unknown[]) => unknown }) => T {
        return ctor =>
            Try.of(() => {
                var ng = ctor.newInstance(entity.getId(), entity.getSystemGenerated(), entity.getVersionKey()) as T;
                return ng;
            }).getOrElseThrow(ex => new RuntimeException(ex));
    }
}

I think an appropriate action item for this issue would be to try to align Prettier Java's formatting of chained method invocations with lambda parameters more closely with Prettier TypeScript's.

xenoterracide commented 6 months ago

Yeah that looks closer (If not the same?) As what I said I was expecting.

jhaber commented 6 months ago

I think the difference is that the .getOrElseThrow isn't line-broken.

xenoterracide commented 6 months ago

is this consistent with typescript?

       var result = ImmutableFindCustomerCommand
            .builder()
            .correlationId(UUID.randomUUID().toString())
            .build()
            .apply(customerApi)
            .block();

again, I thought that formatter did

       var result = ImmutableFindCustomerCommand.builder()
            .correlationId(UUID.randomUUID().toString())
            .build()
            .apply(customerApi)
            .block();

thinking this is just another example of this same issue.

P.S. Not to be an impatient jerk... is this ticket something you think will be fixed within the next few weeks, or is it a ways off. Only asking because I'm implementing prettier where java is the primary concern, and if anyone asks I'd love to have an answer if asked.

xenoterracide commented 4 months ago

Hey, thanks for the good work, I suspect the PR fixed this, but in case it didn't, I figured I'd share it now. Checkstyle alerted me. Although, I'm not certain if it's actually complaining about this issue, it thinks the indentation is wrong on line ... well 3 of the markdown excerpt. I have to refactor this code anyways. Might be a 2nd issue here though.

      Supplier<String> gitVersion = () ->
        Optional
          .ofNullable(porcelainGit.describe())
          .map(v -> v.substring(1))
          .map(v -> v.contains("g") ? v + "-SNAPSHOT" : v)
          .orElse(null);

https://github.com/xenoterracide/gradle-semver/blob/e2049e72c075ea66cbcc0a1e5ca64820a57399ec/src/main/java/com/xenoterracide/gradle/semver/SemVerPlugin.java#L54-L59