PRESUBMIT check for JavaScript style guide violations

[Tyler Breisacher]

I've noticed we don't have any PRESUBMIT rules that check for
violations of the JavaScript style guide. (http://www.chromium.org/
developers/web-development-style-guide#TOC-JavaScript). I asked on IRC
if there were any good tools for checking your JavaScript style
(JSLint is good, but unfortunately the Chromium/Google style guide
differs from Crockford's in a lot of places) and was pointed to
gjslint, which is part of Closure Linter (http://code.google.com/p/
closure-linter/). Since PRESUBMIT.py and Closure Linter are both
written in Python, it seems like it would make sense to use Closure
Linter as a library to check for JavaScript style guide violations,
and then we could filter out the errors that don't apply to us (for
example, use of |const| is okay since we don't have to worry about
cross-browser concerns). Based on a quick run of gjslint.py, it looks
like there is a lot of code already in place that Closure Linter
doesn't like, so we would probably want to check for errors only on
new lines of code.

I'd like to add this to our PRESUBMIT checks, so I wanted to see if
anyone had already attempted this or something like it before, or had
some words of wisdom about doing this kind of thing.

[Demetrios Papadopoulos]

I did run gjslint recently in chrome/browser/resources/print_preview/,
see http://codereview.chromium.org/8605001/. I would like to have this
as a presubmit check. But if we choose do to so I would prefer having
it for all js code not just new lines of code (I think this would
increase complexity, while fixing existing code should not be that
time consuming). OWNERS of each webui feature should take
responsibility of fixing the style errors before we enable the
presubmit check.

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
>    http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

On Fri, Jan 13, 2012 at 5:30 PM, Demetrios Papadopoulos
<dpa...@chromium.org> wrote:
> I did run gjslint recently in chrome/browser/resources/print_preview/,
> see http://codereview.chromium.org/8605001/. I would like to have this
> as a presubmit check. But if we choose do to so I would prefer having
> it for all js code not just new lines of code (I think this would
> increase complexity, while fixing existing code should not be that
> time consuming). OWNERS of each webui feature should take
> responsibility of fixing the style errors before we enable the
> presubmit check.

^ If you do this, please make sure that it doesn't run for CLs that
touch no .js files, to not make presubmits slower for everyone.

Nico

[Jamie Walch]

On a related note, I recommend that Javascript authors run the closure
compiler as a presubmit sanity check. We started doing that for all JS
under remoting/ a few months ago and it's saved us more than once.

[Demetrios Papadopoulos]

I like that idea a lot, having used the closure compiler for other
projects. It is so nice catching silly errors during compilation. I
wanted to take it a step further and allow using the Closure library
in our WebUI but that is another discussion. I can briefly recall from
a conversation that people considered this when WebUI was just
starting and rejected it. If you have any more info feel free to send
to this thread or me (It might be a bit offtopic).

Thank you.

[Dominic Mazzoni]

One reason not to use the Closure library is because it is, among other things, a cross-browser compatibility library full of tricks to work around other browser bugs, whereas most Chrome JavaScript code is written using the latest JS and HTML5 features.

However, using the Closure compiler simply as a presubmit check - to catch coding errors - would be a fantastic idea.

- Dominic

[PhistucK]

Just a quick note - using "const" is not OK.

It is not performant and the V8 team advises against it.

There is actually a filed issue for removing "const" from the code.

☆PhistucK

[Tyler Breisacher]

Thanks for the feedback everyone. I've uploaded a patch at
https://chromiumcodereview.appspot.com/9288045/ -- any feedback is
welcome. A couple of things to note:

1. closure_linter is not part of our codebase so you'll have to
install it (http://code.google.com/closure/utilities/docs/
linter_howto.html). Of course we'll have to add it as a dependency
before this lands.

2. I looked through the list of errors (http://code.google.com/p/
closure-linter/source/browse/trunk/closure_linter/errors.py) and (now
that I know const is frowned-upon) I didn't see any we could disable
other than COMMA_AT_END_OF_LITERAL. We might want to turn off some of
the JSDoc ones too, I'm not sure.