Add eslint for source code

[IanVS]

I think @Globegitter has been pretty busy lately, so this is an offer to implement just the linting part of #935. I am willing to go through and try to fix failing rules, but I would like to get consensus on what the rules should be prior to spending the effort. With the rules from #935 (currently in this PR), I get the following results:

(Note that all these screenshots are cropped on the right, a few bars are much longer)

Another strategy to avoid bikeshedding and arguing over rules, would be to use eslint-config-standard. With that set of rules, these are the results:

My suggestion would be to override the following rules to more closely align with the existing codebase:

"rules": {
  "space-before-function-paren":  [2, "never"],
  "space-after-keywords":         [2, "never"],
  "semi":                         [2, "always"]
}

The results of these three changes results in something pretty manageable:

What say ye?

[IanVS]

I've made sure that all linting rules are warnings, such that this could be merged at any time and will not break the tests. I still would recommend agreeing on the rules before going forward with updating the code.

[dmarcelino]

👍

I'm not too familiar with eslint rules, I've been using jshint and pretty much stick with the defaults. @IanVS do you have any opinions about which rules we should change in regards to the defaults?

[IanVS]

ESLint has pretty minimal "default" rules, and even those will be going away in the upcoming 1.0 release. It bills itself as unopinionated. The "JavaScript Standard Style" is a pretty good set of rules I think. Perhaps what I can do is make a sample commit for each of the rules, with one change, so you can see the diff and decide if you like it. I'm not too familiar with the waterline code base, so it's hard for me to say which rules make the most sense for you.

[IanVS]

The commits named "Fix ESLint Rule" are the easy ones (mechanical, not much thought needed, not an overwhelming number of them).

For the rest of the changes, I'll do a few examples as a "preview" and ask for guidance before continuing.

[dmarcelino]

I'm good with these changes.

But would like hear other people's opinions such as @devinivy, @tjwebb and @particlebanana.

[devinivy]

I think it's great 👍

[IanVS]

Would you like me to squash these commits before proceeding to the slightly-more-difficult changes? Or, I'm happy to leave them separate, which I think makes it very clear to see what was done for each rule (both now and in the future).

[IanVS]

Please take a look at the commit messages labeled "Example" and give your feedback on whether to proceed with fixing the code as I started, or whether we should tweak the rules more to your liking.

There are a handful of other rules I did not attempt to fix. These are:

9 errors: new-cap: [1, { "newIsCap": true, "capIsNew": false }]
7 errors: no-proto: 1
13 errors: eqeqeq: [1, "allow-null"]
3 errors: no-new: 1

I think these are all pretty reasonable, and they can be left in a warning state until the Waterline team can examine and fix them, or turn the rules off if they can't be fixed. I just don't have the knowledge of your code needed to fix those four rules.

[dmarcelino]

@IanVS, the PR is good for now. Lets see if we can get some feedback from @particlebanana or @tjwebb.

[IanVS]

Hopefully there can be a review soon, so I don't need to rebase with master and then make even more changes to fix new linting errors. 😉

[particlebanana]

@IanVS I'm looking over this today, not familiar with ESLint so I'm looking over it and all the rules we have here.

[IanVS]

@particlebanana Awesome, thanks. It can take a little time to research the rules, but I've found the docs to be quite good. The configuration I'm proposing here mostly follows https://github.com/feross/eslint-config-standard/blob/master/eslintrc.json, with a few exceptions as mentioned. If you have any questions, let me know and I'll be happy to try to answer.

[particlebanana]

This is awesome! Thanks @IanVS and I'm ashamed of some of the stuff in there!

[dmarcelino]

Haha, lets work on getting a nice set of rules that make sense to us, fix all issues and then make rules return errors when broken! :)

Thanks @IanVS!

[IanVS]

Great, glad this is useful. Here are what I see as the decisions to be made:

• no-multiple-empty-lines
  • 1 empty line
  • 2 empty lines
• space-after-keywords
  • Always
  • Never
• space-before-function-paren
  • Always
  • Never
  • Always for anonymous, never for named

For now I can work on the other rules like one-var, no-unused-vars, and brace-style. Once decisions are reached on the above, I can work on those too. The following will need to be addressed by the Waterline team:

• no-undef
• new-cap
• no-proto
• eqeqeq
• no-new

Now that this is merged, linting will occur on each test run, but it will look a little overwhelming. You can also use npm run nibble to see rule summaries and then focus in on a specific error.

[dmarcelino]

• no-multiple-empty-lines
  • 1 empty line
  • 2 empty lines

Given how abundantly empty lines are used in waterline I vote for allowing 2 lines (this will still allow just one line, right?).

[IanVS]

Yes, it just means max of two sequential empty lines.

[devinivy]

I vote,

• no-multiple-empty-lines
  • 1 empty line
  • 2 empty lines
• space-after-keywords
  • Always
  • Never
• space-before-function-paren
  • Always
  • Never
  • Always for anonymous, never for named

[particlebanana]

I vote:

no-multiple-empty-lines: 2
space-after-keywords: always
space-before-function-paren: never
no-undef: true
new-cap: true
no-proto: true
eqeqeq: true - smart
no-new: true

[tjwebb]

I vote for one max empty line. We can split the different and make it 1.5? :)

[dmarcelino]

I'm afraid your math is off:

@dmarcelino : 2
@devinivy: 2
@particlebanana: 2
@tjwebb: 1

Average: 1.75 😛