Code review comment for lp:~yellow/launchpad/accordion-client-1

Revision history for this message
Benji York (benji) wrote :

> Whew, done. There are 20-odd items that need addressing but most of them
> are quite small.
[snip]
> One thing that I did notice was that certain things (like the
> trailing-comma-before-closing-brace problem, see [6]) should have been
> picked up by our JS linter (which I think runs as part of `make lint.`
> Could you re-run that for me and, if they're not being picked up, file a
> bug?

The JS linter was indeed not functioning. I filed bug 742619 which has
already provoked a pocketlint fix to be released.

In response to the broken linter I used Crockford's JSLint to guide me
in revision 12660.

A large number of missing semicolons and extra commas were
inserted/removed.

All == and != were changed do === and !==, respectively.

Most uses of string subscripts (foo['bar']) were changed to attribute
access (foo.bar).

Loops over arrays were changed from using the for-in syntax to instead
use traditional indexed for loops.

Added a few missing "var" declarations.

Instead of writing up an exhaustive justification for these changes I'll
let Mr. Crockford explain: http://javascript.crockford.com/code.html

I'll address each of the non-lint comments in a subsequent note.

« Back to merge proposal