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

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

Closed
xenoterracide opened this issue Dec 19, 2023 · 7 comments · Fixed by #632
Closed

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

xenoterracide opened this issue Dec 19, 2023 · 7 comments · Fixed by #632

Comments

@xenoterracide
Copy link

xenoterracide commented Dec 19, 2023

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
Copy link
Contributor

jhaber commented Dec 19, 2023

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
Copy link
Author

xenoterracide commented Dec 19, 2023

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
Copy link
Contributor

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
Copy link
Author

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

@jhaber
Copy link
Contributor

jhaber commented Dec 20, 2023

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

@xenoterracide
Copy link
Author

xenoterracide commented Dec 28, 2023

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
Copy link
Author

xenoterracide commented Feb 17, 2024

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

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 a pull request may close this issue.

3 participants