Gulp format not behaving as expected

[bryceosterhaus]

This commit metal/metal.js@b467094 is entirely from the gulp format task. There are some strange results from the formatter that we should figure out, specifically indentation issues. See metal/metal.js@b467094#diff-8613848b3fa31f88bb889f2483e95900R937 for reference

[jbalsas]

Might be worth to take a look into https://github.com/jlongster/prettier for this?

In technical terms: prettier parses your JavaScript into an AST and pretty-prints the AST, completely ignoring any of the original formatting. Say hello to completely consistent syntax!

[bryceosterhaus]

@jbalsas, it looks promising. However, it is very very new so I would be hesitant to adopting something so quickly. I thought the discussion on this issue was very helpful though, prettier/prettier#40, especially when it comes to all sorts of Liferay formatting standards.

I do love the idea of having something that will auto-format in the editor though, it would be very useful and one less thing to worry about while coding.

[jbalsas]

Hey @bryceosterhaus, reading through the updated Google JavaScript Style Guide I noticed this appendix in the 9.3 Style-related tools section. Seems like we could use ClangFormat for this.

It has:

• Brew clang-format formula
• gulp-format and gulp-clang-format gulp plugins. The latter seems to be the one used in the Angular project
• Visual Studio Extension
• Sublime Text 3 Extension

[julien]

Hi @bryceosterhaus and @jbalsas,

I wanted to suggest ESLint to the possible options, it has a --fix option which can reformat the code too.

[eduardolundgren]

To summarize what we've been discussing our JavaScript projects must follow Google JavaScript Style Guide, and it can be enforced by the usage of the Google eslint config.

We can use prettier to auto-format the code based on Google rules (or at least as close as possible).

  gulp.task('format', () => gulp.src('src/**/*.js')
    .pipe(prettier({
      bracketSpacing: false,
      singleQuote: true,
      trailingComma: 'all',
    }))
    .pipe(gulp.dest('src')));

Everything seemed fine after formatting with prettier, there are few options on ESLint that need to be disabled.

{
  "extends": ["eslint:recommended", "google"],
  "parserOptions": {
      "ecmaVersion": 2017,
      "sourceType": "module"
  },
  "env": {
    "es6": true
  },
  "rules": {
    "arrow-parens": 0, // disabled because of prettier
    "comma-spacing": 0, // disabled because of prettier
    "no-console": 0,
    "no-process-env": 0
  }
}

There is one enforcement that prettier does that is incompatible with Google Style Guide, the usage of semi-colon. Seems like prettier will add support to it prettier/prettier#736. After that we are fine to go with this configuration.

Let me know if you guys are fine with this setup. Thank you!

/cc @fernandosouza @jbalsas Could you guys keep an eye on prettier support for semi-colon and once we have that we can migrate gulp-metal and all our projects to this configuration.

[fernandosouza]

Sure, @eduardolundgren. I'm following the issue and the repo as well.

[jbalsas]

Hey @eduardolundgren, I'm not sure I understand the semicolon issue.

Google Style Guide states that:

Every statement must be terminated with a semicolon. Relying on automatic semicolon insertion is forbidden. [*]

[*] 4.3.2 Semicolons are required

Other than that, I'm pretty happy with this setup. Should we also try to enforce the usage of a Pre-commit hook?

[eduardolundgren]

@jbalsas Yes, prettier doesn't require semicolons, in reality it removes the semicolon, that's why they will add an option for that.

[jbalsas]

I actually think the rather enforce it very strictly... https://github.com/prettier/prettier/blob/b91d6384a6db769789ecd332cdf3b26e1e4d59c2/src/printer.js#L958-L959

See this gist

I understand the issue to be the opposite... people asking to be able to write without semicolons. Where did you see them being removed?

[eduardolundgren]

I would disable more roles:

{
  "extends": ["eslint:recommended", "google"],
  "parserOptions": {
      "ecmaVersion": 2017,
      "sourceType": "module"
  },
  "env": {
    "es6": true
  },
  "rules": {
    "arrow-parens": 0, // disabled because of prettier
    "comma-spacing": 0, // disabled because of prettier
    "no-empty": 0, // disable because we would like to allow empty blocks
    "no-console": 0,
    "no-process-env": 0
  }
}

[bryceosterhaus]

Any updates with this? Prettier released v1.1 recently and it seems to work pretty well, I added it to both devtools and css-transitions using it through eslint-plugin-prettier.

[jbalsas]

Hey @bryceosterhaus, I sent #47 a while ago. I need to update it now that 1.0 has been released and useTabs is available 😉

We also have a guidelines document we plan to present as soon as possible with this and other related guidelines.