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

format differences with PrettierJS #218

Closed
Hawkurane opened this issue Jun 18, 2019 · 9 comments
Closed

format differences with PrettierJS #218

Hawkurane opened this issue Jun 18, 2019 · 9 comments
Labels
area: bug 🐛 Something isn't working status: PR done The PR to fix this issue is done theme: Re-Writer

Comments

@Hawkurane
Copy link
Contributor

Hawkurane commented Jun 18, 2019

We want to stay as close as possible to the reformatting style of Prettier JS

Below are some examples that should be reviewed:

  • TODO
var databaseSizeBeforeCreate = userRepository.findAll().collectList().block().size();
// PrettierJS =>
var databaseSizeBeforeCreate = userRepository
  .findAll()
  .collectList()
  .block()
  .size();
// Prettier-Java =>
var databaseSizeBeforeCreate = userRepository.findAll()
  .collectList()
  .block()
  .size();
  • TODO
var LONGER_LONGER_LONGER_LONGER_LONGER_LOCALE_REQUEST_ATTRIBUTE_NAME = AngularCookieLocaleContextResolver.class.getName() + ".LOCALE";
// PrettierJS =>
var LONGER_LONGER_LONGER_LONGER_LONGER_LOCALE_REQUEST_ATTRIBUTE_NAME =
  AngularCookieLocaleContextResolver.class.getName() + ".LOCALE";

private static final String LOCALE_REQUEST_ATTRIBUTE_NAME = AngularCookieLocaleContextResolver.class.getName()+ ".LOCALE";
// Prettier-Java =>
private static final String LOCALE_REQUEST_ATTRIBUTE_NAME = AngularCookieLocaleContextResolver
  .class
    .getName()
    + ".LOCALE";

// It seems prettier tries to put the second variable on a second line and then only try to break the second variable if it still exceeds the 80 character line
if(aVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName && anotherVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName)
{ // do something
}
// PrettierJS =>
if (
  aVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName &&
  anotherVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName
) {
  // do something
}
// Prettier-Java =>
if (aVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName
  && anotherVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName) { // do something
}
@clementdessoude
Copy link
Contributor

I put it here, as I think the change has to be done in the same place. We have also this bug :

public static void main(String[] args) {
    if(test == 1 && test == 1 && test == 1 && test == 1 && test == 1 && test == 1 && test == 1 && test == 1) {

    }
}

Converted in

public class Args {

  public static void main(String[] args) {
    if (test
      == 1
      && test
      == 1
      && test
      == 1
      && test
      == 1
      && test
      == 1
      && test
      == 1
      && test
      == 1
      && test
      == 1) {}
  }
}

@Hawkurane
Copy link
Contributor Author

What should the formatting be?

@DanielFran
Copy link
Member

It should break at 80 characters (or other line limit characters)?

@Hawkurane
Copy link
Contributor Author

Hawkurane commented Jun 19, 2019

It should, but I'm unsure what to look for, maybe something like this instead?

public class Args {

  public static void main(String[] args) {
    if (test == 1
      && test == 1
      && test == 1
      && test == 1
      && test == 1
      && test == 1
      && test == 1
      && test == 1) {}
  }
}

@clementdessoude
Copy link
Contributor

clementdessoude commented Jun 19, 2019

In js, it is printed like that:

function HelloWorld() {
  if (
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1
  ) {
  }
}

but we can move the && to the next line

@DanielFran
Copy link
Member

Probably a mix of both result like:

function HelloWorld() {
  if (test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1 &&
    test === 1) {
  }
}

@bd82
Copy link
Contributor

bd82 commented Jun 19, 2019

but we can move the && to the next line

Is it common to put && on the next line in Java?

Probably a mix of both result like:

@DanielFran I think the example by @clement26695 is slightly clearer, but regardless of my own personal opinion on the topic, should not we closely match the style of prettier-js simplify because it is much more mature and thus we would likely avoid pitfalls and issues that they already resolved?

@jhaber
Copy link
Contributor

jhaber commented Sep 30, 2019

but we can move the && to the next line

Is it common to put && on the next line in Java?

Just wanted to chime in that putting the && on the next line is in fact common in Java. Many companies (including us) piggyback on the Google code style for Java which uses this pattern:
https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break

When a line is broken at a non-assignment operator the break comes before the symbol. (Note that this is not the same practice used in Google style for other languages, such as C++ and JavaScript.)

Not a dealbreaker for us, but if there was an option to put these operators on the next line we would use that option

@clementdessoude clementdessoude added area: bug 🐛 Something isn't working status: PR done The PR to fix this issue is done theme: Re-Writer labels Oct 2, 2019
@clementdessoude
Copy link
Contributor

Closed with #255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: bug 🐛 Something isn't working status: PR done The PR to fix this issue is done theme: Re-Writer
Projects
None yet
Development

No branches or pull requests

5 participants