fixes #1608: integrate eslint into testing process

[michaelficarra]

This generates the following warnings, but all errors (and tons of other warnings) have been eliminated.

underscore.js
    41:37  warning  A constructor name should start with an uppercase letter         new-cap
   361:17  warning  Gratuitous parentheses around expression                         no-extra-parens
   930:12  warning  Expected '!==' and instead saw '!='                              eqeqeq
   930:28  warning  Expected '!==' and instead saw '!='                              eqeqeq
   932:15  warning  Expected '===' and instead saw '=='                              eqeqeq
   932:24  warning  Expected '===' and instead saw '=='                              eqeqeq
   932:41  warning  Expected '===' and instead saw '=='                              eqeqeq
  1049:2   warning  Unexpected constant condition                                    no-constant-condition
  1234:26  warning  This function has too many parameters (5). Maximum allowed is 4  max-params
  1259:19  warning  The Function constructor is eval                                 no-new-func

The next version of eslint (current master) will allow comments for line-based exceptions so we can mark all of these as expected.

Fixes #1608.

[jdalton]

comments for line-based exceptions

Oh joy! 😩

Would something like node-jscs be useful or an alternative?

[braddunbar]

No need for node_modules/.bin/ here since npm test adds that directory to the path.

[michaelficarra]

@jdalton: I don't see how having comments for exceptions is bad. There will be 7 in total, and they make sense. We have one use of the Function constructor, and we're okay with that. But we'd want any other uses to trigger test failure. And I chose eslint over jscs because its rules are much more powerful, checking for potential errors as well as enforcing style.

@braddunbar and @davidchambers: I addressed your comments. Thanks for the review.

[jdalton]

I don't see how having comments for exceptions is bad. There will be 7 in total, and they make sense. We have one use of the Function constructor, and we're okay with that. But we'd want any other uses to trigger test failure.

Ya, it's just not my thing. I don't like magic comments (for builds, linting, etc.). I don't get value from that. I'm also not a fan of causing builds to error if coding style is not aligned because I don't want to burden those contributing with minor nits and would rather accept a PR and push a followup cleanup commit than get bogged down in the nits (reduce barriers to contributions where possible).

My biggest concern, accidental globals, is already covered with the existing global checks that QUnit provides.

[braddunbar]

Comments for exceptions leaves a bad taste in my mouth as well. If it's not a rule we intend to follow, it seems that we should just turn it off or leave the warnings. Otherwise, I think the changes here are fine.

[akre54]

Agreed fully with @jdalton here. Underscore has an active corps of maintainers who have been more than capable of policing the project's style. Little things like accidental globals might sneak through every once in a while, but I'm more interested in finding out why Travis didn't catch it and warn against a merge.

[davidchambers]

LGTM

[jdalton]

@akre54

but I'm more interested in finding out why Travis didn't catch it and warn against a merge.

It actually did fail the travis-ci job and failure was visible in the PR. See #1602.
It may help to enable email notification (for my own projects I like being pinged on runs).

[akre54]

Nah email was way too obnoxious for me (and I can only imagine what it
would be for Jeremy).The github hooks shouldve caught this and stopped a
merge.

[jdalton]

Nah email was way too obnoxious for me (and I can only imagine what it
would be for Jeremy)

And yet somehow I manage and things like accidental globals don't go unnoticed in my projects.

The github hooks should've caught this and stopped a
merge.

They did there job. It was merged even with the fail. Adding more unnecessary process via this PR doesn't solve that.

[jridgewell]

Should these be reversed? With @jdalton's comment on not burdening contributors, I'd be much more interested in a travis build passing tests and failing the linter instead of it failing the linter and never making it to the tests.

[megawac]

How about normalizing the new Array(n) construct vs Array(n). Personally, I prefer the latter

[michaelficarra]

@megawac: I've taken care of that separately here: dcd668a

[michaelficarra]

By the way, as more incentive to merge this, I just used eslint as configured here to discover #1641. Working on rebasing this off master now.

[jdalton]

Unused var detection is handy.

[michaelficarra]

This PR is ready for review again.