google / eslint-config-google

ESLint shareable config for the Google JavaScript style guide
Apache License 2.0
1.74k stars 140 forks source link

Update all rules to match the new styleguide #23

Closed philipwalton closed 7 years ago

philipwalton commented 8 years ago

@addyosmani PTAL

I've gone through the new JavaScript styleguide (unfortunately still not public) and updated all ESLint rules to match (there's a lot of them, so we might want to have some other folks double-check).

As I say in the comments in index.js, I think the easiest way to keep this in sync with ESLint is to list all rules and comment out the ones that the Google styleguide doesn't have an opinion on.

I've opted not to extend the default eslint:recommended ruleset, though I anticipate lots of projects will want to do that, and they still can with something like this:

{
  "extends": ["eslint:recommended", "google"]
}

There are a few rules in the Google styleguide that current ESLint rules can't fully enforce. As we have time going forward, we can add our own rules to handle some of those specific cases. As one quick example: Google style enforces braces around all conditional statements, but it makes an exception for short if-statements like the following:

function foo() {
  if (shortConditional()) return;

  // ...
}
gauntface commented 8 years ago

What was the reasoning for not extending 'eslint:recommended'? What happens for rules Google has no opinion on and we aren't extending eslint:recommended?

For consistency I would want rules to be consistent throughout a project, sounds like without extending, there will be a lot of room for several developers to add different styles on the rules that aren't defined.

Other than that - LGTM from me.

Should remove 0.10 and 0.12 from travis. ESLint is failing tests for me on other projects where they are dropping support.

philipwalton commented 7 years ago

What was the reasoning for not extending 'eslint:recommended'?

The main reason is I didn't want anyone to mistakenly get the impression that Google's JS style guide enforces/recommends a particular rule/pattern when in fact it's ESLint that enforces it.

I also wanted to give developers the option to only use Google style if they really want. If we extend eslint:recommended, developers would have to manually unset any rule they don't want to follow.

What happens for rules Google has no opinion on and we aren't extending eslint:recommended?

Nothing happens. They just don't get a warning if they break one of ESLint's recommended rules.

For consistency I would want rules to be consistent throughout a project, sounds like without extending, there will be a lot of room for several developers to add different styles on the rules that aren't defined.

Most of the ESLint's recommended rules aren't stylistic, they fall under the possible errors category. The only stylistic one is no-mixed-spaces-and-tabs, which Google also requires (by requiring spaces).

Also, FWIW, I imagine, for most of our projects, we'll want to extend eslint:recommended because those rules are generally good to follow, even if Google doesn't explicitly enforce them.

Should remove 0.10 and 0.12 from travis. ESLint is failing tests for me on other projects where they are dropping support.

I added a commit that removed those versions.

addyosmani commented 7 years ago

The main reason is I didn't want anyone to mistakenly get the impression that Google's JS style guide enforces/recommends a particular rule/pattern when in fact it's ESLint that enforces it. I also wanted to give developers the option to only use Google style if they really want. If we extend eslint:recommended, developers would have to manually unset any rule they don't want to follow.

I talked this over with @gauntface offline. Imo, your approach makes sense and I like the idea of avoiding folks having to unset rules.

addyosmani commented 7 years ago

There are a few rules in the Google styleguide that current ESLint rules can't fully enforce. As we have time going forward, we can add our own rules to handle some of those specific cases.

Post PR, it would be great to capture a rough list of what rules can't be enforced. I feel like ESLint is extensible enough for some exploratory work to be done here but 👍 on looking at better handling other rules moving forward.

philipwalton commented 7 years ago

Post PR, it would be great to capture a rough list of what rules can't be enforced. I feel like ESLint is extensible enough for some exploratory work to be done here but 👍 on looking at better handling other rules moving forward.

100% agree! And I'm happy to do the exploring.

addyosmani commented 7 years ago

100% agree! And I'm happy to do the exploring.

👍