coding style

[boneskull]

I'm unable to really get a handle on it, and what with all the PRs over the years, the codebase seems pretty inconsistent.

We have leading commas in some places, not in others, semi-colons here, no semi-colons there, blocks here and no blocks there.

I'm proposing: let's clean it up!

We could go whatever direction, but in the interest of full disclosure my coding style is different than TJ's. My style aligns pretty closely with Felix's Style Guide, but I'm more in the "Crockford" camp than he is.

Some differences, none of which I am willing to go to war over:

• I declare all my vars at the top of each function, as one statement, each separated by newlines + commas.
• I tend to use foo_bar for a variable name, and fooBar for a function name
• I tend to write ternary statements on one line unless they are hairy.
• I use JSDoc docstrings where appropriate, instead of always using slashes
• I will use setters.

However...

I would strongly prefer to always use semi-colons, because it is more friendly to developers who don't know how to not use semi-colons. (We get a lot of PRs.)

I would strongly prefer using curly braces in conditionals and loops, because I don't trust myself to write a complex "if/else if/else if/else" conditional without them.

I strongly prefer not to using leading commas, because it is untoward and impure.

We can set up .jshintrc and go with that; add a jshint target to the Makefile. Usually I have one for production code, and another for test code (which will have different global vars/functions). We can specify browser: true and node: true and solve the issue of having Mocha run in dual contexts.

Also an .editorconfig would be helpful.

As far as actually reformatting things, I use PyCharm which is pretty smart about it, and could turn this around pretty quickly.

So my questions are:

1. Should we enforce a style?
2. If so, should we use Felix's Style Guide?
3. Some other guide?
4. Anything style-related you feel strongly about?

[jbnicolai]

Should we enforce a style?

Yes, big +1 on this.

If so, should we use Felix's Style Guide?
Some other guide?

Like you, I don't feel strongly. I agree that braces for conditionals and semi-colons to separate statements are a good idea to make the codebase more approachable.

Anything style-related you feel strongly about?

Anything that we agree on should be enforceable. I suggest using both jshint for error prevention and jscs for consistent style.

If we don't make npm test fail on incorrect style I fear it would be too hard to check pull-requests for adherence.

[boneskull]

Yes, I tend to always make jshint result part of my npm test

[rlidwka]

If we don't make npm test fail on incorrect style I fear it would be too hard to check pull-requests for adherence.

And if you do, you'll just be scaring away contributors with travis errors. The thing is: travis can't distinguish between real errors and linter complaints, it all looks the same. And when you see "build failed" when there isn't anything wrong with the code, that's annoying.

"+1" for editorconfig however. It's completely optional, and it's useful if you have an editor that supports it.

[boneskull]

And if you do, you'll just be scaring away contributors with travis errors. The thing is: travis can't distinguish between real errors and linter complaints, it all looks the same. And when you see "build failed" when there isn't anything wrong with the code, that's annoying.

The code is wrong inasmuch as it fails the lint check, which means it's still wrong. I won't merge linty code into the codebase, and I don't want to hand-edit a bunch of linty PRs.

There's no point in attempting to adhere to a style if it's not enforced in some way.

If/when we do this, the lint check should be part of npm test. Anybody who is contributing to Mocha is expected to run npm test before sending a PR (this should be formalized in a CONTRIBUTING.md; see #882).

"Scaring away contributors" is far-fetched. I fail to see how expecting contributors to submit code as per any project's guidelines is too much to ask. This is basic OSS etiquette.

[ilanbiala]

+1 for .editorconfig

[dasilvacontin]

+1 for enforcing the coding style.

[gtramontina]

If using jshint, you might want to take a look at https://github.com/jshint/fixmyjs -- I haven't used it myself, but it seems to give you a good head start.

[boneskull]

@gtramontina Thanks for the link. I've also been experimenting recently with JSCS and have had good results. So maybe a combination of JSCS + JSHint would be cool.

[dasilvacontin]

Bump. I'd vote to go with JSHint, and start deciding the code style rules. :)

[ainthek]

With JSHint, defaults (empty .jshintrc)

267 Bad line breaking before ','.
29 Comma warnings can be turned off with 'laxcomma'.
23 Bad line breaking before '+'.
16 Use '===' to compare with '0'.
15 Missing semicolon.
15 Bad line breaking before '?'.
13 The 'proto' property is deprecated.
11 Unnecessary semicolon.
11 Missing '()' invoking a constructor.
10 Expected an assignment or function call and instead saw an expression.
8 Bad line breaking before '||'.
5 Redefinition of 'Date'.
3 Use '!==' to compare with 'null'.
3 A leading decimal point can be confused with a dot: '.75'.
3 'el' used out of scope.
2 'err' is already defined.
2 'el' is already defined.
1 ['long'] is better written in dot notation.
1 Use the function form of "use strict".
1 Use '!==' to compare with 'undefined'.
1 Unmatched '{'.
1 Unmatched '('.
1 Too many errors. (57% scanned).
1 Too many errors. (25% scanned).
1 Expected a conditional expression and instead saw an assignment.
1 Expected '(end)' and instead saw '}'.
1 Don't make functions within a loop.
1 A leading decimal point can be confused with a dot: '.50'.
1 'tag' is already defined.
1 'stat' used out of scope.
1 'output' is already defined.
1 'options' is already defined.
1 'oldPos' is already defined.
1 'module' is not defined.
1 'i' is already defined.
1 'fmt' is already defined.

with Felixes it seems even more:

243 Bad line breaking before ','.
66 Expected '{' and instead saw 'return'.
62 Line is too long.
51 Expected '===' and instead saw '=='.
29 Comma warnings can be turned off with 'laxcomma'.
23 Bad line breaking before '+'.
13 The 'proto' property is deprecated.
13 Strings must use singlequote.
13 Missing semicolon.
12 Bad line breaking before '?'.
10 Missing 'new' prefix when invoking a constructor.
9 Expected '{' and instead saw 'this'.
8 Expected an assignment or function call and instead saw an expression.
8 Expected '!==' and instead saw '!='.
8 Bad line breaking before '||'.
6 Missing '()' invoking a constructor.
6 Expected '{' and instead saw 'throw'.
6 Expected '{' and instead saw 'fn'.
5 Unnecessary semicolon.
5 Redefinition of 'Date'.
4 This function has too many statements. (16)
4 Expected '{' and instead saw 'path'.
4 Expected '{' and instead saw 'ms'.
4 Blocks are nested too deeply. (4)
3 Expected '{' and instead saw 'runner'.
3 Expected '{' and instead saw 'process'.
3 Expected '{' and instead saw 'continue'.
3 Expected '{' and instead saw 'console'.
3 A leading decimal point can be confused with a dot: '.75'.
2 Expected '{' and instead saw 'try'.
2 Expected '{' and instead saw 'suites'.
2 Expected '{' and instead saw 'progress'.
2 Expected '{' and instead saw 'mocha'.
2 Expected '{' and instead saw 'match'.
2 Expected '{' and instead saw 'hideSuitesWithout'.
2 'el' is already defined.
1 ['long'] is better written in dot notation.
1 Unmatched '{'.
1 Unmatched '('.
1 Too many errors. (96% scanned).
1 Too many errors. (79% scanned).
1 Too many errors. (78% scanned).
1 Too many errors. (52% scanned).
1 Too many errors. (48% scanned).
1 Too many errors. (4% scanned).
1 This function has too many statements. (44)
1 This function has too many statements. (36)
1 This function has too many statements. (22)
1 This function has too many statements. (17)
1 Redefinition of 'global'.
1 Expected a conditional expression and instead saw an assignment.
1 Expected '{' and instead saw 'write'.
1 Expected '{' and instead saw 'trajectory'.
1 Expected '{' and instead saw 'total'.
1 Expected '{' and instead saw 'test'.
1 Expected '{' and instead saw 'tag'.
1 Expected '{' and instead saw 'suite'.
1 Expected '{' and instead saw 'str'.
1 Expected '{' and instead saw 'self'.
1 Expected '{' and instead saw 'ret'.
1 Expected '{' and instead saw 'result'.
1 Expected '{' and instead saw 'opts'.
1 Expected '{' and instead saw 'err'.
1 Expected '{' and instead saw 'ctx'.
1 Expected '{' and instead saw 'bail'.
1 Expected '(end)' and instead saw '}'.
1 Don't make functions within a loop.
1 A leading decimal point can be confused with a dot: '.50'.
1 A constructor name should start with an uppercase letter.
1 'tag' is already defined.
1 'output' is already defined.
1 'options' is already defined.
1 'oldPos' is already defined.
1 'fmt' is already defined.
1 'err' is already defined.

excluded node_modules and tests.

I suggest starting with empty (default jshint, make first cleanup, and then fine tune)

[ainthek]

Mocha.js itself have this list:

mocha.js: line 7, col 7, Comma warnings can be turned off with 'laxcomma'.
mocha.js: line 6, col 33, Bad line breaking before ','.
mocha.js: line 19, col 16, Bad line breaking before ','.
mocha.js: line 20, col 22, Bad line breaking before ','.
mocha.js: line 23, col 7, Bad line breaking before '||'.
mocha.js: line 24, col 7, Bad line breaking before '||'.
mocha.js: line 35, col 34, Bad line breaking before ','.
mocha.js: line 53, col 4, Missing semicolon.
mocha.js: line 158, col 24, 'oldPos' is already defined.
mocha.js: line 301, col 110, Don't make functions within a loop.
mocha.js: line 320, col 19, Bad line breaking before '+'.
mocha.js: line 321, col 19, Bad line breaking before '+'.
mocha.js: line 372, col 18, 'i' is already defined.
mocha.js: line 467, col 26, Unnecessary semicolon.
mocha.js: line 505, col 4, Unnecessary semicolon.
mocha.js: line 729, col 45, Bad line breaking before ','.
mocha.js: line 730, col 21, Bad line breaking before ','.
mocha.js: line 731, col 23, Bad line breaking before ','.
mocha.js: line 732, col 13, Bad line breaking before ','.
mocha.js: line 733, col 13, Bad line breaking before ','.
mocha.js: line 734, col 22, Bad line breaking before ','.
mocha.js: line 755, col 46, Bad line breaking before ','.
mocha.js: line 759, col 9, Bad line breaking before ','.
mocha.js: line 760, col 21, Bad line breaking before ','.
mocha.js: line 808, col 9, Use '===' to compare with '0'.
mocha.js: line 901, col 15, Unnecessary semicolon.
mocha.js: line 903, col 22, Missing '()' invoking a constructor.
mocha.js: line 916, col 9, Use '===' to compare with '0'.
mocha.js: line 917, col 13, 'err' is already defined.
mocha.js: line 932, col 31, Bad line breaking before ','.
mocha.js: line 933, col 29, Bad line breaking before ','.
mocha.js: line 934, col 31, Bad line breaking before ','.
mocha.js: line 1075, col 31, Bad line breaking before ','.
mocha.js: line 1148, col 31, Bad line breaking before ','.
mocha.js: line 1149, col 29, Bad line breaking before ','.
mocha.js: line 1150, col 54, Bad line breaking before ','.
mocha.js: line 1276, col 31, Bad line breaking before ','.
mocha.js: line 1277, col 29, Bad line breaking before ','.
mocha.js: line 1278, col 54, Bad line breaking before ','.
mocha.js: line 1426, col 34, Bad line breaking before ','.
mocha.js: line 1427, col 54, Bad line breaking before ','.
mocha.js: line 1441, col 19, Bad line breaking before ','.
mocha.js: line 1495, col 50, Missing '()' invoking a constructor.
mocha.js: line 1499, col 12, Use '!==' to compare with 'null'.
mocha.js: line 1500, col 36, Missing semicolon.
mocha.js: line 1528, col 9, Use '===' to compare with '0'.
mocha.js: line 1558, col 75, Unnecessary semicolon.
mocha.js: line 1559, col 74, Unnecessary semicolon.
mocha.js: line 1581, col 66, Unnecessary semicolon.
mocha.js: line 1602, col 29, Expected an assignment or function call and instead saw an expression.
mocha.js: line 1602, col 29, Too many errors. (25% scanned).

so once you agree, I can spent some time fixing them and retesting.
To work on reporters, makes sense only once all this is agreed.

[ainthek]

or we can start linting by default only main file: _mocha .js, please see:

https://github.com/ainthek/mocha/commit/0d98a3b00132999668b88c835dbde6438110deea

[ainthek]

How do you want to approach garbage included from externals ?
like this one ?

lib/browser/diff.js: node_modules/diff/diff.js
cp node_modules/diff/diff.js lib/browser/diff.js

[dasilvacontin]

Ignore files from other modules, imo.

[jbnicolai]

Closing, since #1750 has now been merged.