Automate style checking for javascript files

Registered by Imre Farkas on 2013-08-06

Horizon currently doesn't have any coding standards for javascript files. We need to have an agreement over one and then automate style checking for js, just like pep8 does for python. It also involves fixing all the inconsistencies in the existing codebase and making changes according to the accepted coding standards.

Blueprint information

Status:
Complete
Approver:
David Lyle
Priority:
Medium
Drafter:
Imre Farkas
Direction:
Needs approval
Assignee:
Radomir Dopieralski
Definition:
Approved
Series goal:
Accepted for icehouse
Implementation:
Implemented
Milestone target:
milestone icon 2014.1
Started by
Imre Farkas on 2013-08-09
Completed by
David Lyle on 2013-12-17

Related branches

Sprints

Whiteboard

[ifarkas] There are many style guides available. An article [1] by Seravo tries to find a consensus among them, it would make sense for Horizon to adapt it.

As for the automation, integrating a tool like JSHint [2] could do the automatic style checking. We have multiple options:
- similarly to pep8, we can extend the run_tests.sh to run JSHint and print the output as JSHint can be run as a Node.js application
- we can integrate JSHint with the existing qUnit javascript test framework.
I propose the later as it doesn't introduce a dependency to Node.js and has an advantage of running automatically with the selenium tests.

[1] https://github.com/Seravo/js-winning-style
[2] http://www.jshint.com/

Gerrit topic: https://review.openstack.org/#q,topic:bp/js-coding-style-checks,n,z

Addressed by: https://review.openstack.org/41087
    JSHint integration

mrunge, Nov 20 2013: since it took us long to get rid of node.js, I wouldn't vote for anything bringing us node.js back, even not through the backdoor.

rdopieralski, 2013-11-25: I'm all for adding style checks for JavaScript, but if we are going to do it, we need to think about the roadmap for bringing the style up to par with the checks first. I don't think that adding the style fixes together with the test check in one huge commit is a feasible approach. Another option is to fix all the style problems in a chain of patches, and rebase the addition of the test at the end of that chain -- but it's going to be a hell merging it with any changes that pop up in js in the mean time. I think that the best approach is to add the jshint to tests with all the failing checks disabled initially, and then fix the issues and enable the checks gradually in subsequent patches. This way we don't have to make one huge change, but at the same time we make sure that the things that we gradually fix remain fixed.

[jpichon, 2013-11-25] This makes sense and would match the way we've been enabling the HACKING tests. ( https://blueprints.launchpad.net/horizon/+spec/hacking )

Gerrit topic: https://review.openstack.org/#q,topic:js-style-extra-zeros,n,z

Addressed by: https://review.openstack.org/58529
    Use === and !== instead of == and != in JavaScript.

Addressed by: https://review.openstack.org/58550
    Use [] for new arrays in JavaScript

Addressed by: https://review.openstack.org/58570
    Fix confusing use of ! and = in JavaScript

Addressed by: https://review.openstack.org/58546
    Fix duplicate definition and scope in JavaScript

Addressed by: https://review.openstack.org/58576
    Use dot notation in JavaScript

Addressed by: https://review.openstack.org/58563
    Fix duplicate keys in JavaScript

Addressed by: https://review.openstack.org/58573
    Fix bad line breaking and radix in JavaScript

Addressed by: https://review.openstack.org/58542
    Fix semicolons in JavaScript

Addressed by: https://review.openstack.org/58535
    Don't use extra leading zeros in JavaScript

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.