Style guide and automatic enforcement of it

[ckarlof]

Studies suggest code style consistency makes code easier to read and developers more productive. I'd like a style guide and an automatic way to enforce it, when possible via jshint, jscs (jscs-dev/node-jscs#102), or whatever.

Mozilla Webdev style guide: http://mozweb.readthedocs.org/en/latest/js-style.html#js-style

[ckarlof]

It would also be nice if it was as similar as possible to the style guide for fxa-auth-server (mozilla/fxa-auth-server#472).

[nchapman]

We do have the beginnings of this going with the jshint grunt task: grunt jshint. We just need to settle on the proper style rules. I believe what we're using now is pretty much what came with Yeoman.

[shane-tomlinson]

I have added a pre-commit hook to do the hard work before every commit.

The following pre-commit hook, when added to fxa-content-server/.git/hooks/pre-commit, will run jshint before commit:

#!/bin/sh

exec grunt jshint

[shane-tomlinson]

Link to #174

[pdehaan]

Another option is ESLint nzakas/eslint which is fairly configurable and a bit easier to customize warnings/errors. Also has support for Grunt and other stuff.

[pdehaan]

I came up with the following .jscs.json config file, but I don't see any setting for ASI, so we still may have to use JSHint Grunt task to fill in some gaps in the Mozilla Style Guide:

{
    "disallowKeywords": ["with", "eval"],
    "disallowKeywordsOnNewLine": ["else"],
    "disallowLeftStickedOperators": ["?", "-", "/", "*", "=", "==", "===", "!=", "!==", ">", ">=", "<", "<="],
    "disallowMultipleLineStrings": true,
    "disallowRightStickedOperators": ["?", "/", "*", ":", "=", "==", "===", "!=", "!==", ">", ">=", "<", "<="],
    "disallowSpaceAfterObjectKeys": true,
    "disallowSpaceAfterPrefixUnaryOperators": ["++", "--", "+", "-"],
    "disallowSpaceBeforePostfixUnaryOperators": ["++", "--"],
    "maximumLineLength": 120,
    "requireCapitalizedConstructors": true,
    "requireCurlyBraces": ["if", "else", "for", "while", "do"],
    "requireLeftStickedOperators": [","],
    "requireLineFeedAtFileEnd": true,
    "requireRightStickedOperators": ["!"],
    "requireSpaceAfterKeywords": ["if", "else", "for", "while", "do", "switch", "return"],
    "requireSpaceBeforeBinaryOperators": ["+", "-", "/", "*", "=", "==", "===", "!=", "!=="],
    "validateLineBreaks": "LF",
    "validateQuoteMarks": true,
    "validateJSDoc": {
        "checkParamNames": true,
        "checkRedundantParams": true,
        "requireParamTypes": true
    }
}

You can test this locally by installing jscs [1.2.0] globally and running $ jscs */.js and you should see about 4 errors/warnings:

$ jscs **/*.js
If statement without curly braces at scripts/run_locally.js :
    26 |  fxaccntbridge.on('exit', function(code, signal) {
    27 |    console.log('fxa-content-server killed, existing');
    28 |    if (done) done(code);
------------^
    29 |    else process.exit(code);
    30 |  });

Else statement without curly braces at scripts/run_locally.js :
    26 |  fxaccntbridge.on('exit', function(code, signal) {
    27 |    console.log('fxa-content-server killed, existing');
    28 |    if (done) done(code);
------------^
    29 |    else process.exit(code);
    30 |  });

If statement without curly braces at scripts/run_locally.js :
    33 |// only start the server if the file is called directly, otherwise wait until
    34 |// module.exports is called.
    35 |if (process.argv[1] === __filename) module.exports();
--------^
    36 |
    37 |

Keyword `else` should not be placed on new line at scripts/run_locally.js :
    27 |    console.log('fxa-content-server killed, existing');
    28 |    if (done) done(code);
    29 |    else process.exit(code);
------------^
    30 |  });
    31 |};

4 code style errors found.

Note that this config file won't work with the grunt-jscs-checker package until they update the jscs dependency from 1.1.0 to 1.2.0 (I have a pending PR). Once that gets merged I can submit a PR to wire up the jscs Grunt task.

Also note that when/if we make changes to this config file, it should be copied to the other 2-3 fxa-* repos to keep things consistent.

[shane-tomlinson]

I suggest we add a run of whichever linter we decide on to the travis build to catch errors like #219.

[pdehaan]

Initial commit for this in PR #226

[pdehaan]

I think this was fixed a long time ago. Closing.