Enforce specific style guide

[mucaho]

Recent efforts into improving our code style (e.g. #949) got me thinking. Why don't we enforce a specific code style and never worry about the code style being messed up by accident again?

There is a tool that does this exactly - JSCS. We can use a preset style guide, or customize our own according to a set of rules. There is also an accompanying grunt task.

This will probably require a lot of work in the start to fix all outstanding issues in our code, but once we overcome this initial hurdle, it should hopefully be worthwhile in the long run (since tests will fail unless style is accepted). Oh boy, it even supports automatic formatting (to some degree I guess) to match the code style we want!

We actually have a style guide we could enforce (and perhaps update it with more rules that make sense).

[mucaho]

Also I recently encountered an issue with a variable accidentally leaking into global namespace and corrupting a for loop in my code (this one case is fixed with fe26b9d).

JSHint will with next release support implicit use of strict statements (see jshint/jshint#924), so we can add that and more optional features.
Basically you get "use strict" safety of e.g. global variable leaks without explicitly adding these "use strict" statements all around the place. We can then use the strictify browserify-plugin node module to add those "use strict" statements to release bundles.

[mucaho]

JSHint's strictest opt-in options, what's currently enforced or the reason it isn't:

• "bitwise": true, reason: there are a few places that need them
• "curly": true, reason: introduces syntactical overhead, better check correct indentation with JSCS
• "eqeqeq": true,
• "esversion": 5,
• "forin": true, reason: performance optimization, may not be faster than iteration over Object.keys, needs profiling
• "freeze": true,
• "futurehostile": true,
• "latedef": true, changed to "nofunc": using functions before they are declared is the main reason to use function declarations imo
• "noarg": true,
• "nocomma": true, reason: it's a nice shortcut, used in e.g. for (var i = 0, l = array.length; i < l; ++i)
• "nonbsp": true,
• "nonew": true,
• "plusplus": true, reason: nope
• "singleGroups": true, reason: unnecessary parentheses may sometimes improve readability
• "strict": "global", changed to "implied": test tools (see footnote) have bugs with "use strict" statements, fix this in future to improve performance slightly
• "undef": true,
• "unused": "strict", changed to "vars": there are two use-cases:
  1. api functions: there should be no unused parameters, regardless of position, should be set to "strict"
  2. callbacks: "strict" option can not be used with callbacks, therefore only "true" possible
  3. However, as unused function parameters come in handy to show signature of callbacks and "true" doesn't prevent all unused parameters of api functions, the option is set to check only "vars" for now
  4. Note that this could be changed to "true" and unused callback parameters then commented out /*, unusedParam*/ to achieve somewhat better checks

Footnote:
phantomjs < 2.0 has problems with "use strict" and arguments, see issue, this is fixed in 2.0.
Once the phantomjs lib in grunt-contrib-qunit upgrades to 2.0+, add "use strict"; statements to the top of every file.

[starwed]

I've heard good things about prettier, which will automatically maintain a standard format. It is more narrowly scoped (format only, e.g. it won't warn you of things like unused variables) and less configurable than linting tools, so it might be easier to start using with the existing code base.

From their readme:

Unlike ESLint, there aren't a million configuration options and rules. But more importantly: everything is fixable. This works because Prettier never "checks" anything; it takes JavaScript as input and delivers the formatted JavaScript as output.

In technical terms: Prettier parses your JavaScript into an AST (Abstract Syntax Tree) and pretty-prints the AST, completely ignoring any of the original formatting. Say hello to completely consistent syntax!

I might take a pass of seeing what happens when running it against crafty.

[starwed]

I might take a pass of seeing what happens when running it against crafty.

The results seemed mostly reasonable, with some weirdness about docblock indenation. (Found a related issue in their repo, and it sounds like it is not the intended behavior.)

Here's the result of running it against our src+tests directories.

[mucaho]

Sounds good. JSHint handles static analysis, this could compliment it well. In the long run it might be a problem that prettier doesn't offer many configuration option, but is very good due to automatic formatting. ESLint offers great config options, but can't format everything and will fail the build for every single indentation mishap.

I'd suggest to tweak the options though. Imo let's try and see how something like this looks (source files are easier to read with 4 spaces indentation).

  useTabs: false,
  tabWidth: 4,
  printWidth: 106,
  singleQuote: true,
  trailingComma: "none",
  bracketSpacing: true,
  semi: true

However, aside from doc blocks, it spits out monstrosities like this for example. (e: Or does this have to do with max line length?) If the effort to // prettier-ignore these exceptions is relatively small I'm on board!

[starwed]

The particular monstrosity you mention is already pretty hard to read; it should probably be refactored to do less calculations in line. (e.g. var scrollTop = (doc && doc.scrollLeft) || (body && body.scrollLeft) || 0). I suspect that most especially ugly blocks will be the result of code that's illegible already! :)

[starwed]

ESLint offers great config options, but can't format everything and will fail the build for every single indentation mishap.

Right, I think the way we'd want to use this is to add the git commit hook that autoformats on staging; that way the committed code is always formatted consistently.