Enforce coding style

[williamboman]

Closes #485.

To lint; $ npm run lint.

[astorije]

Hi @williamboman, thanks a lot, that's big work!!
Give me some time to read about ESLint and give the first review (not in CONTRIBUTING which is in writing stage now, but we do require 2 👍 for a merge).

However, take this time to try to convince me with ESLint :-)
I am myself very familiar with JSCS (as you can see here and there) and even started to contribute on it (one pending PR that I need to give some love to). When I look at your configuration file and mine, I much prefer JSCS as its configuration is very much human readable, ESLint's one is pretty cryptic to the neophyte IMO.
However, I will agree that ESLint "lints" while JSCS only checks style (in which case we'd need a .jshintrc file as well), but it checks it extremely thoroughly.
I know that I have a skewed opinion and yours will be as skewed as mine, but both trying to be objective, what do you think? I will make some research about ESLint and consider it the same way I would consider JSCS :-)

[williamboman]

@astorije I've actually been using both jshint and jscs together for quite some time. I find ESLint to be easier to configure. When needing both a code checker and style checker ESLint tends to be quicker than jshint & jscs combined, so that's a pro.

They both achieve the same thing, and the configuration files themselves are really just touched once - or very seldom, so to me it really doesn't matter.

[williamboman]

What about strict mode? It's missing everywhere right now.

[astorije]

@williamboman, thanks for giving your objective opinion. I will give a read to ESLint to take an informed decision. What I can see so far is that my JSCS configuration files seem much more rigorous in terms of style choices than your ESLint file. At first glance, I'd say that JSCS can better avoid confusion and set consistent style, but again, that might be because I don't know about it (in particular, the "extends": "eslint:recommended" sounds promising and maybe answers my concern). Again, I'll read and get back to you.

For strict mode (and other things), I fully agree with you, but let's set the minimal coding style rules as part of this PR to have the project comply with them, and then we can make the entire project stricter by adding relevant rules (including stricting everything). Makes sense?

[olivierlambert]

What about using standard? (just my 2 cents)

[williamboman]

@olivierlambert The existing code in this repo does not adhere to standard, so no 😄.

Personally, I don't like some of the standard rules (enough to never consider using it).

[olivierlambert]

That was just a suggestion ;) I fully understand some efforts and trade-off to comply to standard.

[JocelynDelalande]

@williamboman could you rebase on master ? there are conflicts.

@astorije are you ok with the proposed changes as-is (after rebase of course) ? It would be good to merge such a PR soon cause it's quite conflicts-prone...

I'm myself not very acustomed to js-style issues, so just looking at that from a distant point of view (but with great joy to see coding standards incoming !)

[JocelynDelalande]

@williamboman would'nt it be good to run those linters in before_script in .travis.yml ?

[astorije]

Please leave me some more time on this. I will get to it for sure.

[williamboman]

Rebased

edit: @astorije As I said before, if you feel unsure about eslint, feel free to provide a JSHint and JSCS configuration and I'll replace it 😄.

[astorije]

Thanks @williamboman!

I'm still reading ESLint docs, give me a few more days and I should be back on track for this.

@JocelynDelalande, actually, it's even better to put it in the script section so that tests run no matter what and break or pass independently.
But don't worry about this at the moment @williamboman, we'll add it once this is merged to spare you potential other rebases.

Stay tuned.

[floogulinc]

What about using https://github.com/sindresorhus/xo?

[williamboman]

What about using https://github.com/sindresorhus/xo?

I personally feel pretty meh about xo. It's comfortable for small projects.

[astorije]

This one should go, but I don't want to add extra delay to your PR. Could you add the comment # TODO Fix trailing commas and remove this line at the end of this line please?

[williamboman]

Do you want it to be disallowed altogether? Having it enabled for objects that span multiple lines is pretty nice for diffing stuff.

[astorije]

I don't have a super strong opinion at the moment, so disregard my comment for this PR. However, I think we'll have to remove this for compatibility reasons.

[williamboman]

Trailing commas already exist in the code base, all non-discontinued major browser (well, except IE8 which will be discontinued in January next year) supports it as well.

I really like trailing commas, as it makes cleaner diffs like these possible for adding/removing stuff;

diff --git a/foo.js b/foo.js
index 9c78d86..eb26d74 100644
--- a/foo.js
+++ b/foo.js
@@ -2,4 +2,5 @@ var foo = [
    "foo",
    "bar",
    "quux",
+   "baz",
 ];

vs

diff --git a/foo.js b/foo.js
index 67d2cbd..2aada49 100644
--- a/foo.js
+++ b/foo.js
@@ -1,5 +1,6 @@
 var foo = [
    "foo",
    "bar",
-   "quux"
+   "quux",
+   "baz"
 ];

[Xe]

The typical haskell style is to do:

myList :: [String]
mylist = [ "foo"
         , "bar"
         , "baz"
         ]

I have seen this repeated in many JS projects in the past and it works for them.

This also means that adding things to the list doesn't change more than one line of code and that it is impossible to end up with a trailing comma.

[astorije]

@williamboman, that's fair comment. I should have specified that I had no strong opinion on the choice, but that I'd want one style to be enforced, either trailing commas everywhere, or nowhere (an alternative could be, everywhere on the server, nowhere on the client, until IE8 is not being used).
Let's keep comma-dangle: 0 and make a broader decision later, in a future PR (but I'd go in your direction now, I think).

[astorije]

Yeah, @Xe, some are using that horrifying syntax indeed.
Thankfully, as all fashions kid like to wear, it is losing its popularity :-)
Let's not do that, and anyway the current codebase is not using crazyness like this, so even better.

---

"Closing" (virtually) this line of comments as decision was reached: we keep comma-dangle: 0 for the moment.

[williamboman]

Thankfully, as all fashions kid like to wear, it is losing its popularity :-)

Afaik, that convention is extremely ubiquitous in Haskell 😄.

[astorije]

Actually, this is very painful in practice. One cares more about failing tests than failing style rules.

Adding this in Travis's script has the advantage of running all tasks, even in case of failure. But it means developers need to remember to run npm test and npm run lint. The former one is common practice by Node's users (npm run test is aliased by npm test, Travis CI runs npm test automatically, ...).

Therefore, could you remove this line from .travis.yml and update the test task in package.json accordingly:

"test": "HOME=test/fixtures mocha test/**/*.js && npm run lint",

Best of both worlds.

[astorije]

@williamboman, I finally found time to go through the entire PR. This is neat, thank you very much for your hard work!! I see plenty room for improvement, but let's get this merged quickly and the improvements will go in further PRs.

2 comments however before I can give my definite 👍:

• Address my comment about before_script. To give some contexts, I worked on 3 Node projects where linter tasks were defined in a before_script or as the first step of the test task. In all 3 cases, it kept being in our way: Travis CI would not tell us right away if tests were passing because style was failing, or we could not do stupid changes locally and have tests running.
• Move the first comment about .editorconfig in a different PR. I cannot give my comments about it without reading about it and it's a shame this is blocking this current PR.

Are you OK with these changes? :-)

Thanks again!

[williamboman]

Move the first comment about .editorconfig in a different PR. I cannot give my comments about it without reading about it and it's a shame this is blocking this current PR.

Sorry, what do you mean?

[astorije]

Sorry, I meant first commit, not first comment...

[williamboman]

Done

[astorije]

Great, thanks! Here is my 👍 then :-)

@JocelynDelalande, still OK with this? Could you give yours please?

[JocelynDelalande]

Ok, that's really cool to read all this harmonized code, just finished reading your comments and the PR, 👍 Great work.

Sorry for reviewing delay