Convert from JSHint to ESLint

[pdehaan]

ESLint is a bit more flexible and has one great advantage of shareable configs, which allows you to publish your ESLint rules to npm (ie: eslint-config-gulp) and then share those rules across all your gulpjs/* repos in GitHub.

[tunnckoCore]

better would be to follow "standard style" or something widely used.

[phated]

I have been thinking about converting to eslint. Will review this later.

@tunnckoCore Standard won't be followed here.

[pdehaan]

I just converted the existing .jshintrc files to .eslintrc (using polyjuice). But yeah, I'm happy to switch to standard or airbnb style guide, although it'll probably require a bunch of tweaking to code to get it passing again.

[phated]

@pdehaan we follow https://github.com/felixge/node-style-guide

[pdehaan]

@phated Great!

I copied the .eslintrc file (and had to tweak a deprecated ESLint rule -- sadly it doesn't look like the rules are published to npm), and got 2 errors and 1 warning in the gulpjs/gulp repo:

{
  "extends": "eslint:recommended",
  "env": {
    "node": true
  },
  "rules": {
    "array-bracket-spacing": [2, "never"],
    "block-scoped-var": 2,
    "brace-style": [2, "1tbs"],
    "camelcase": 1,
    "comma-dangle": 0,
    "curly": 2,
    "eol-last": 2,
    "eqeqeq": [2, "smart"],
    "max-depth": [1, 3],
    "max-len": [1, 80],
    "max-statements": [1, 15],
    "new-cap": 1,
    "no-console": 0,
    "no-extend-native": 2,
    "no-mixed-spaces-and-tabs": 2,
    "no-trailing-spaces": 2,
    "no-unused-vars": 1,
    "no-use-before-define": [2, "nofunc"],
    "object-curly-spacing": [2, "never"],
    "quotes": [2, "single", "avoid-escape"],
    "semi": [2, "always"],
    "space-after-keywords": [2, "always"],
    "space-unary-ops": 2
  }
}

Output:

➜  $ eslint .

bin/gulp.js
  75:1  warning  This function has too many statements (24). Maximum allowed is 15  max-statements

test/watch.js
  127:11  error  watcher was used before it was defined  no-use-before-define
  166:11  error  watcher was used before it was defined  no-use-before-define

✖ 3 problems (2 errors, 1 warning)

---

Using the same .eslintrc file, it looks like there are 5 warnings in the gulpjs/gulp-cli repo:

➜  $ eslint .

index.js
  79:1  warning  This function has too many statements (24). Maximum allowed is 15  max-statements

test/flags-task-simple.js
   9:1  warning  Line 9 exceeds the maximum line length of 80   max-len
  14:1  warning  Line 14 exceeds the maximum line length of 80  max-len

test/flags-tasks.js
  11:1  warning  Line 11 exceeds the maximum line length of 80  max-len

test/flags-version.js
  14:1  warning  Line 14 exceeds the maximum line length of 80  max-len

✖ 5 problems (0 errors, 5 warnings)

Not sure if you want to disable the max-len rule, or why JSHint wasn't catching those, or if we just ignore that rule in the test/ directories using a nested .eslintrc file.

[pdehaan]

OK, made a few more tweaks and published eslint-config-node-style-guide to npm.

The current /.eslintrc file looks like this, where the ESLint rules slightly differed from the node-style-guide:

extends: node-style-guide

rules:
  indent: [2, 2]
  max-statements: [2, 50]
  new-cap: 0
  no-eq-null: 2
  no-unused-vars: [2, {vars: all, args: after-used}]
  strict: [2, global]

[callumacrae]

I opened an issue asking @felixge to push node-style-guide to npm, which should remove the need for your npm package: felixge/node-style-guide#69

[callumacrae]

@pdehaan Are you planning on pushing node-style-guide to npm now it has a package.json and updating this PR?

[pdehaan]

Oh, I was planning on converting eslint-config-node-style-guide to use the felixge/node-style-guide version of the .eslint file. But last I checked it was throwing warnings because of outdated eslint rules (see felixge/node-style-guide#71).

I'll poke around a bit more today and see if I can find a way to make it work with latest ESLint.

[phated]

Thanks. Also, can we get the eslintrc in json? Not a fan of yaml

[phated]

@pdehaan can you please update this to JSON instead of yaml? Thanks

[pdehaan]

@phated YAML has been JSON-ified, dependencies bumped, and repushed.

[pdehaan]

Had to override this "node-style-guide" rule since it was causing all (9x) these errors:

/Users/pdehaan/dev/github/gulp/test/watch.js
  1:1  error  Rule 'space-in-brackets' was removed and replaced by: object-curly-spacing, array-bracket-spacing, computed-property-spacing  space-in-brackets

See https://github.com/felixge/node-style-guide/pull/71/files

[phated]

It really stinks that felix's repo is unmaintained. I wonder if we should switch to something like https://www.npmjs.com/package/xo (cc'ing @sindresorhus)

The reason I bring this up is that there is considerable working going into making all of these repos (and all the undertaker stuff) the same and having to copy the .eslintrc from here into each project seems exhausting. It would have been so much nicer if we could have just used "extends": "node-style-guide".

@contra what do you think about xo?

@sindresorhus how does xo work with Sublime? I assume sublime-linter eslint plugin doesn't pick up the config?

[yocontra]

@phated xo looks cool, I'm down with using it as long as long as it doesn't radically change from our current style

[pdehaan]

I ran eslint-config-xo against this repo w/ eslint-nibble and here's a summary of the errors:

$ npm run nibble

> gulp@3.9.0 nibble /Users/pdehaan/dev/github/gulp
> eslint-nibble .

indent:                      514|
comma-dangle:                 15|
space-before-function-paren: 125|
global-require:                2|
padded-blocks:                10|
no-warning-comments:           1|
max-nested-callbacks:          5|
no-unused-expressions:         1|
no-inline-comments:            4|
one-var:                       6|

9 files checked.  0 passed.  9 failed.  8 warnings.  675 errors.

[yocontra]

It's kind of an undertaking (no pun intended) but we should implement the same style guideline and project template across all of our repos once we decide what we want to do

[yocontra]

@pdehaan Did you set it to allow spaces instead of tabs? That's probably where most of those are coming from

[pdehaan]

It looks like this config should get you back to 0 errors/warnings:

{
  "extends": "xo",
  "rules": {
    "comma-dangle": [2, "always-multiline"],
    "global-require": 0,
    "indent": [2, 2],
    "max-nested-callbacks": 0,
    "no-console": 0,
    "no-inline-comments": 0,
    "no-unused-expressions": 0,
    "no-warning-comments": 0,
    "one-var": 0,
    "padded-blocks": 0,
    "space-before-function-paren": 0
  }
}

Then we could publish that as eslint-config-gulp and share it between the repos.

[pdehaan]

@contra The eslint-config-xo config sets the indent rule to tabs instead of spaces by default (source).

[sindresorhus]

You could use the space option in XO or ESLint with eslint-config-xo-space (no point in extending eslint:recommended as the XO config has all of that).

I would recommend you consider adapting to these rules instead of changing them as they're there for a reason, to make your code more explicit and less prone to bugs, except for the space/tab thing ofc.

Using XO directly is very nice, though, as you almost don't need any config at all. This is how it would look:

{
    "scripts": {
        "test": "xo"
    },
    "devDependencies": {
        "xo": "^0.9.0"
    },
    "xo": {
        "space": true
    }
}

And you can add XO simply by running $ xo --init in your project.

how does xo work with Sublime? I assume sublime-linter eslint plugin doesn't pick up the config?

If you use XO, there's the SublimeLinter-contrib-xo plugin. With the eslint-config-xo-space config, you can use SublimeLinter-eslint.

[phated]

I agree that we should change the style to match XO (except spaces) if we are going to switch. I need to review the things we would need to change.

[phated]

Just ran xo --space against the CLI and i agree with a bunch of the stuff but there is a few things that I don't. Maybe we just need a consistent eslint shared config for gulp repos.

[sindresorhus]

@phated I'm interested in hearing what you don't agree with regardless of what you choose.

[phated]

@sindresorhus inconsistently quoted properties, inline comments, unexpected require warning (not your fault - would be really nice if eslint could see require is being used with a variable instead of string), object-curly-spacing on single key, inline objects (currently disallowed)

[phated]

@pdehaan I've added you as a collaborator on https://github.com/gulpjs/eslint-config-gulp to help us get a fleshed out ruleset for the gulp repos.

[sindresorhus]

Just want to explain my reasoning: (I'm always happy to be questioned about it)

inconsistently quoted properties

I was unsure about it at first. It took some time to get used to, but I think my code is a lot better with it. If one property needs to be quoted, it's no longer a normal object, but an hashmap in my view, and keys should be treated as strings, not variable names. E.g. with headers from a request.

inline comments

I'm still not sure about this one. It has benefited readability a bit.

unexpected require warning

It's to better prepare for ES2015 modules where everything needs to be imported at the top. It's only a warning, though. And I agree, ESLint should not warn if it's not a string. Should at least be an option for it. Open an issue → https://github.com/eslint/eslint/issues

object-curly-spacing on single key

It's inconsistent to have spacing there, but nowhere else. To be consistent you should also do [ 'foo' ] and x( 'foo' ) in that case. I think the spacing there is moot anyways as syntax higlighting makes the separation clear.

inline objects

What does this mean?

[phated]

@sindresorhus https://github.com/gulpjs/gulp-cli/blob/4.0/lib/versioned/%5E4.0.0-alpha.1/index.js#L44 is the example of a single-key inline object. Sorry about the comma, didn't think about that when typing it; It wasn't meant to separate as another item :P

I disagree that your array and function example are consistent because neither of them defines a key-value pair, only value.

[sindresorhus]

Ok, fair enough. As for https://github.com/gulpjs/eslint-config-gulp, it should be easy to just depend on and extends eslint-config-xo-space and change those 4 rules.

[pdehaan]

It really stinks that felix's repo is unmaintained.

On a bright note, I was just added as a contributor to that repo, so I can go merge happy.

That said, now that we have ESLint and JSCS presets, I can get back to this PR and rebase against master and hopefully get it landed.

[erikkemperman]

@pdehaan @phated
I was looking into making some gulp#4.0 repos (undertaker, bach, async-*, ...) consistent with respect to eslint. Guess I'd better wait until this lands and then use it as my starting point. But I notice that those other repos currently don't depend on jscs; do we need/prefer to have both here?

[pdehaan]

I was only adding a standard gulp/jscs workflow to things in the gulpjs/* repos.
If there are other repos that you want me to try adding these to, we should make a list/gist and I can try running through them like a checklist. Or if they are core to Gulp, maybe consider moving them into the gulpjs/* GitHub org.

[phated]

@pdehaan I moved them last night. I was going to open issues on all of them for these housekeeping issues.

[erikkemperman]

I was playing with the 4.0 branch, and the modules I blundered into are not used in gulp master. They are already (mostly?) using eslint, so making them consistent should really only be a matter of dropping in these base rules and then "fixing" the code.

But I just wondered, looking at the rules in .eslintrc from node-style-guide (and its name...), if a separate enforcer for style wouldn't be redundant.

[erikkemperman]

@phated Sorry for off-topic here but about that move: is undertakerjs going to remain separate?

[phated]

@erikkemperman nope, I am going to move those too, just forgot (it was late)

[phated]

@erikkemperman note that https://github.com/js-cli will remain separate

[phated]

@erikkemperman I've added 3 issues to each of the transferred repos for the housekeeping stuff that needs to be done. A repo that has everything done already is https://github.com/gulpjs/vinyl

[pdehaan]

@phated @erikkemperman Not pretty, but I built a dirty scraper at https://gist.github.com/pdehaan/be529766a508b5da65b3 which shows the .eslintrc, .jscsrc, and .travis.yml file for each of the gulpjs/* repos.

[pdehaan]

Rebased.

[erikkemperman]

@pdehaan
Nice! One minor addition maybe, this fetches files from the master branches of these repos. For gulp itself though I guess the 4.0 branch ought to be included as well?

[pdehaan]

@erikkemperman Sure, tweaked to allow for optional branches: https://gist.github.com/pdehaan/be529766a508b5da65b3

Also, output is pretty noisy, so I can suppress any other repos that we may not care about. I excluded gulp/eslint-config-gulp and gulpjs/jscs-preset-gulp from the list since those are just helpers and don't really need an .eslintrc file or Travis configs.

[erikkemperman]

I think that 4.0 branches for gulp and gulp-cli would be good to have in there.

[pdehaan]

Added gulp-cli#4.0 branch

[phated]

Neat 💯

[pdehaan]

[stevemao]

Why is jscs not removed?