PRESUBMIT check for JavaScript style errors

PRESUBMIT check for JavaScript style errors

See https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/97b5dc28d9e5109b/a5bd070bb7f0a4b9

BUG=none
TEST=modify any .js file; `git commit` it; run `git cl presubmit`; look at the errors

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124007

[Dan Beam, 2012-01-31 02:11:04]

have you actually run this to see how much of our code would be even remotely
close to passing?

[Tyler Breisacher (Chromium), 2012-01-31 19:18:49]

The linter actually throws runtime errors for the following files:

chrome/browser/resources/options[2]/chromeos/internet_detail.js -- we're using
'default' as a property but the linter is expecting it to be used in a switch {}
block.

chrome/test/data/serializer_test.js, chrome/test/data/serializer_nested_test.js,
chrome/test/data/serializer_test_nowhitespace.js -- I'm not sure exactly what
the cause of the error is for these three, but maybe we can just rename them to
.json instead of .js since that appears to be what they are.

Anyway, I made a commit that touches every js file other than those five, and
then modified the presubmit script to just print the number of errors in each
file. Results are here: https://gist.github.com/1712265

Obviously, some of these are unimportant (minified files, things in
third_party...) but there are still a lot of errors.

We can either:
1. Try to fix all these errors before enabling this presubmit check.
2. Enable the presubmit check, and encourage everyone to fix these errors as
they come across them.

I guess I'm leaning toward option 2, but I'll also take a look at some of the
files that have zillions of errors and try to see why that's happening.

[Dan Beam, 2012-01-31 21:25:31]

I greatly favor 2 as well.  The reason I ask is that I don't want people to send
you angry emails or just --bypass-hooks all together for those things they don't
deem useful / take too much time.

Perhaps you might want to limit directories that are linted to willing
participants for right now (like, say, chrome/browser/resources/ntp4/ if you can
get estade and I to agree or chrome/browser/resources/option2/ if you can csilv
and jhawkins to agree).

[Tyler Breisacher (Chromium), 2012-02-01 19:04:48]

From skimming through the errors in various files, it looks like most of the
changes are really simple fixes like adding/removing a space. The only ones that
would be a little more involved are the ones related to jsDoc (no documentation
for a function, |@param| or |@return| annotations are missing, etc). There may
be some functions that should have better documentation, but there are
definitely others that don't particularly need it so maybe we'll want to
suppress some of those.

> Perhaps you might want to limit directories that are linted to willing
> participants for right now

I discussed this with scr@ yesterday and he suggested the same thing. For right
now, I've added the two you suggested, and I'll send out an email to those
OWNERS.

[Tyler Breisacher (Chromium), 2012-02-03 02:14:28]

I wanted to get a glimpse of what life would be like under the oppressive
closure_linter PRESUBMIT regime. So I fixed all the errors it flagged in ntp4
(http://codereview.chromium.org/9320051/) and options2
(http://codereview.chromium.org/9316086/). I found a couple rules that seemed to
be worth ignoring.

Also added a check for document.getElementById (Use $() instead.)

[Dan Beam, 2012-02-03 03:09:13]

https://chromiumcodereview.appspot.com/9288045/diff/8002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/8002/PRESUBMIT.py#newcode311
PRESUBMIT.py:311: results.extend(_CheckJavaScriptStyle(input_api, output_api))
Damn, I guess I should be doing my checks in this file instead of in
depot_tools, d'oh!

[Tyler Breisacher (Chromium), 2012-02-06 20:13:53]

maruel: Dan said you would probably be a good person to review this? I thought I
added you last week but it looks like it didn't work, so I apologize if you are
getting this email twice.

[M-A Ruel, 2012-02-06 20:29:27]

Sorry I missed it.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode16
PRESUBMIT.py:16: import re
This import was an oversight in http://codereview.chromium.org/9233057. Please
move it back inside GetPreferredTrySlaves() and fix other usage of re.foo to
input_api.re.foo.

Move the new imports to be function local too.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode34
PRESUBMIT.py:34: 
2 vertical lines between file level symbols please.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode41
PRESUBMIT.py:41: self._errors = []
if you used self.errors = [], then you wouldn't need GetErrors(). Unless
GetErrors is part of the interface that
closure_linter.checker.JavaScriptStyleChecker() is expecting.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode55
PRESUBMIT.py:55: return not self._errors.empty
What's list.empty?

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode58
PRESUBMIT.py:58: '''Check whether an error is valid. Most errors are valid, with
a few
All the rest of the file use """for docstrings""".

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode82
PRESUBMIT.py:82: return re.search('\.(js|html?)$', affected_file.LocalPath())
input_api.re

[Tyler Breisacher (Chromium), 2012-02-06 22:00:11]

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode16
PRESUBMIT.py:16: import re
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> This import was an oversight in http://codereview.chromium.org/9233057. Please
> move it back inside GetPreferredTrySlaves() and fix other usage of re.foo to
> input_api.re.foo.
> 
> Move the new imports to be function local too. 

Done. This means I have to move ErrorHandlerImpl inside of the
CheckJavaScriptStyle function, which seems weird to me. Is there a less awkward
way of approaching this?

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode34
PRESUBMIT.py:34: 
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> 2 vertical lines between file level symbols please.

Done.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode41
PRESUBMIT.py:41: self._errors = []
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> if you used self.errors = [], then you wouldn't need GetErrors(). Unless
> GetErrors is part of the interface that
> closure_linter.checker.JavaScriptStyleChecker() is expecting.

I think the JavaScriptStyleChecker only calls HandleError and HandleFile, but
HasErrors and GetErrors are part of the ErrorHandler interface:
http://code.google.com/p/closure-linter/source/browse/trunk/closure_linter/co...

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode55
PRESUBMIT.py:55: return not self._errors.empty
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> What's list.empty?

*facepalm*. Thank you! Done.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode58
PRESUBMIT.py:58: '''Check whether an error is valid. Most errors are valid, with
a few
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> All the rest of the file use """for docstrings""".

Done.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode82
PRESUBMIT.py:82: return re.search('\.(js|html?)$', affected_file.LocalPath())
On 2012/02/06 20:29:27, Marc-Antoine Ruel wrote:
> input_api.re

Done.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode93
PRESUBMIT.py:93: for i, line in enumerate(f.NewContents()):
Off by one error! The first line is line 1, not line 0. (Done.)

http://codereview.chromium.org/9288045/diff/13001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/13001/PRESUBMIT.py#newcode66
PRESUBMIT.py:66:
closure_linter.errors.JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER
What is typically done in cases like this to avoid the 80char issue? 'import
closure_linter.errors as cl_errors' or something like that?

[M-A Ruel, 2012-02-07 00:34:09]

lgtm with 2 remaining nits.

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/3006/PRESUBMIT.py#newcode16
PRESUBMIT.py:16: import re
On 2012/02/06 22:00:13, Tyler Breisacher wrote:
> Done. This means I have to move ErrorHandlerImpl inside of the
> CheckJavaScriptStyle function, which seems weird to me. Is there a less
awkward
> way of approaching this?

no. :)

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode66
PRESUBMIT.py:66:
closure_linter.errors.JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER
the presubmit check will be fine since there's a long symbol.
The fix would be to do
   closure_linter.errors.
     JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode70
PRESUBMIT.py:70: import os.path
use input_api.os_path

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode104
PRESUBMIT.py:104: if const_re.search(line):
if input_api.re.search(r'\bconst\b', line):

[Tyler Breisacher (Chromium), 2012-02-07 01:15:59]

Sorry, two more questions:

1. Should the closure_linter library itself live in src/third_party, or in
depot_tools, or somewhere else, and what is the process for adding it?

2. Do we really need to avoid using const? (We've started the conversation on
this at http://code.google.com/p/chromium/issues/detail?id=80149)

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode66
PRESUBMIT.py:66:
closure_linter.errors.JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER
On 2012/02/07 00:34:09, Marc-Antoine Ruel wrote:
> the presubmit check will be fine since there's a long symbol.
> The fix would be to do
>    closure_linter.errors.
>      JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER

Done.

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode70
PRESUBMIT.py:70: import os.path
On 2012/02/07 00:34:09, Marc-Antoine Ruel wrote:
> use input_api.os_path

Done.

http://codereview.chromium.org/9288045/diff/16001/PRESUBMIT.py#newcode104
PRESUBMIT.py:104: if const_re.search(line):
On 2012/02/07 00:34:09, Marc-Antoine Ruel wrote:
> if input_api.re.search(r'\bconst\b', line):

Done.

[M-A Ruel, 2012-02-07 01:17:13]

On 2012/02/07 01:15:59, Tyler Breisacher wrote:
> Sorry, two more questions:
> 
> 1. Should the closure_linter library itself live in src/third_party, or in
> depot_tools, or somewhere else, and what is the process for adding it?

Not yet unless you plan to have it used in another project other than chromium.

M-A
 
> 2. Do we really need to avoid using const? (We've started the conversation on
> this at http://code.google.com/p/chromium/issues/detail?id=80149)

[Dan Beam, 2012-02-07 09:08:00]

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode5
PRESUBMIT.py:5: """Top-level presubmit script for Chromium.
This check should probably not be in src/PRESUBMIT.py, it should be in
src/chrome/browser/resources/PRESUBMIT.py (the same place where mine will go) as
it makes no sense to interpret/run this all the time just to ignore all the
files but those in chrome/browser/resources/*.  If the time comes that we want
to move this to a more common area, seems OK to me, but just my take...

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode30
PRESUBMIT.py:30: """Check for JavaScript style violations."""
Mention that this is checking against our web development style guide and link
to it as we intentionally violate some rules in Chrom{e,ium} as we needn't
support quirks of other browsers.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode32
PRESUBMIT.py:32: import closure_linter.common.errorhandler
Are these actually local imports or is it the same as doing it at the top of the
file?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode80
PRESUBMIT.py:80: def inCheckedFolder(affected_file):
I thought we used in_checked_folder for local functions in python?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode81
PRESUBMIT.py:81: return any(affected_file.LocalPath().startswith(cf)
Is this safe to assume?  What if I have a CL that I've started via svn co
svn://src.chromium.org/chrome/trunk/src/chrome/browser/resources?  I don't have
a particularly good solution for this either at this point, so just wondering...

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode85
PRESUBMIT.py:85: return input_api.re.search('\.(js|html?)$',
affected_file.LocalPath())
Why are you checking HTML?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode93
PRESUBMIT.py:93: errorLines = []
camelCase in JS, under_scores in Python and C++ for locals, as far as I have
even seen

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode96
PRESUBMIT.py:96: for i, line in enumerate(f.NewContents(), start=1):
Whay are you starting at 1?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode97
PRESUBMIT.py:97: if 'getElementById' in line:
nit: I think this should be slightly more robust, IMO (i.e. there are \W chars
on either side or something like that).  In JS you'd need some way of calling it
that'd require .parens() or ['propertyAccessors']() if you're not doing
something really weird, but it's up to you...

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:100: 'Use $() instead of document.getElementById()',
nit: $('id') to show we're not using jQuery (though MooTools and Prototype still
do this...)

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:103: if input_api.re.search(r'\bconst\b', line):
Doesn't closure have something for this?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:117: error.code,
Do you think the average user will care about this error code?

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:131:
'http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml'
This is not necessarily the guide we're following.  You should be linking to and
following the local most (Chromium) style guide first.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:132: ' and contact tbreisacher@chromium.org for feedback on this'
nit: This sounds like they're contacting you for feedback on their run:

  ' and contact tbreisacher@chromium.org with your feedback.'

or something to that extent.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:448: if any(input_api.re.search(aura_re, f.LocalPath())
Where exactly are you getting this input_api variable from?  From the scope
chain?  It's cool that you've removed the dependency on re from this script, but
why not simply fix this to use .find() instead of depending on some magical
unknown variable (or at least document that's where it's coming from).

I don't particularly get why maruel@ wants us both to remove direct dependencies
or imports, as I don't understand how these one time includes (that are already
included elsewhere in this script) are a bad thing as if a script requires the
regex module, it should ... import the regex module.  This is akin to saying
"well, some header file above me #include's <string>, so I don't need to", IMHO.
 maruel@ ^ ?

[Dan Beam, 2012-02-07 09:08:54]

You also probably want to add tests and get OWNERS approval from jhawkins or
csilv for options2 and estade for ntp4.

[M-A Ruel, 2012-02-07 14:16:04]

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode32
PRESUBMIT.py:32: import closure_linter.common.errorhandler
On 2012/02/07 09:08:00, Dan Beam wrote:
> Are these actually local imports or is it the same as doing it at the top of
the
> file?

That's a local import, it's fine.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode80
PRESUBMIT.py:80: def inCheckedFolder(affected_file):
On 2012/02/07 09:08:00, Dan Beam wrote:
> I thought we used in_checked_folder for local functions in python?

Right.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode96
PRESUBMIT.py:96: for i, line in enumerate(f.NewContents(), start=1):
On 2012/02/07 09:08:00, Dan Beam wrote:
> Whay are you starting at 1?

Because line numbers starting at 0 are unintuitive. Note that start=1 was added
in python 2.6 and we still officially support 2.5. But that's fine, I need to
send a PSA about that anyway to deprecate officially 2.5

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:448: if any(input_api.re.search(aura_re, f.LocalPath())
On 2012/02/07 09:08:00, Dan Beam wrote:
> Where exactly are you getting this input_api variable from?  From the scope
> chain?  It's cool that you've removed the dependency on re from this script,
but
> why not simply fix this to use .find() instead of depending on some magical
> unknown variable (or at least document that's where it's coming from).
> 
> I don't particularly get why maruel@ wants us both to remove direct
dependencies
> or imports, as I don't understand how these one time includes (that are
already
> included elsewhere in this script) are a bad thing as if a script requires the
> regex module, it should ... import the regex module.  This is akin to saying
> "well, some header file above me #include's <string>, so I don't need to",
IMHO.
>  maruel@ ^ ?

It's not Tyler's fault. Imports are slow in python. Dan, see
https://goto.google.com/g4presubmit for more infos.

In that case, use a local import.

[Tyler Breisacher (Chromium), 2012-02-08 02:49:28]

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode5
PRESUBMIT.py:5: """Top-level presubmit script for Chromium.
On 2012/02/07 09:08:00, Dan Beam wrote:
> This check should probably not be in src/PRESUBMIT.py, it should be in
> src/chrome/browser/resources/PRESUBMIT.py (the same place where mine will go)
as
> it makes no sense to interpret/run this all the time just to ignore all the
> files but those in chrome/browser/resources/*.  If the time comes that we want
> to move this to a more common area, seems OK to me, but just my take...

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode30
PRESUBMIT.py:30: """Check for JavaScript style violations."""
On 2012/02/07 09:08:00, Dan Beam wrote:
> Mention that this is checking against our web development style guide and link
> to it as we intentionally violate some rules in Chrom{e,ium} as we needn't
> support quirks of other browsers.

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode80
PRESUBMIT.py:80: def inCheckedFolder(affected_file):
On 2012/02/07 09:08:00, Dan Beam wrote:
> I thought we used in_checked_folder for local functions in python?

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode81
PRESUBMIT.py:81: return any(affected_file.LocalPath().startswith(cf)
Good point, I'm not sure what we can do about it either.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode85
PRESUBMIT.py:85: return input_api.re.search('\.(js|html?)$',
affected_file.LocalPath())
It's already equipped to check JS code in <script> tags:
http://code.google.com/p/closure-linter/source/browse/trunk/closure_linter/ch...

I think we generally avoid doing that anyway, so maybe it's not worth it.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode93
PRESUBMIT.py:93: errorLines = []
On 2012/02/07 09:08:00, Dan Beam wrote:
> camelCase in JS, under_scores in Python and C++ for locals, as far as I have
> even seen

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode96
PRESUBMIT.py:96: for i, line in enumerate(f.NewContents(), start=1):
Neither way is particularly more intuitive than the other IMO, it's just that
text editors always use one-based lines.

maruel: Do you want to drop the start argument? We can just print i+1 instead of
i, if you want.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcode97
PRESUBMIT.py:97: if 'getElementById' in line:
I think this is probably okay as is, to be honest. You could even use it without
calling it directly:

elems = listOfIds.map(document.getElementById.bind(document));

so I would put this in the "now you have two problems" category ;)

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:100: 'Use $() instead of document.getElementById()',
On 2012/02/07 09:08:00, Dan Beam wrote:
> nit: $('id') to show we're not using jQuery (though MooTools and Prototype
still
> do this...)

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:103: if input_api.re.search(r'\bconst\b', line):
On 2012/02/07 09:08:00, Dan Beam wrote:
> Doesn't closure have something for this?

You would think so, but no :(

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:117: error.code,
On 2012/02/07 09:08:00, Dan Beam wrote:
> Do you think the average user will care about this error code?

Generally, no, but if this check complains about something you don't understand,
you could Google for "closure linter error 123" and potentially get better
information than if you Googled for "closure linter error unnecessary semicolon"
or whatever. I don't feel strongly about it either way.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:131:
'http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml'
On 2012/02/07 09:08:00, Dan Beam wrote:
> This is not necessarily the guide we're following.  You should be linking to
and
> following the local most (Chromium) style guide first.

Done.

https://chromiumcodereview.appspot.com/9288045/diff/18002/PRESUBMIT.py#newcod...
PRESUBMIT.py:132: ' and contact tbreisacher@chromium.org for feedback on this'
On 2012/02/07 09:08:00, Dan Beam wrote:
> nit: This sounds like they're contacting you for feedback on their run:
> 
>   ' and contact mailto:tbreisacher@chromium.org with your feedback.'
> 
> or something to that extent.

Done.

[M-A Ruel, 2012-02-08 14:30:39]

https://chromiumcodereview.appspot.com/9288045/diff/23002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23002/PRESUBMIT.py#newcode9
PRESUBMIT.py:9: """
Let's remove changes to this file from this CL. I'll do it separately.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:14:
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
if you remove www., it fits 80 cols. :)

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:20: """Implementation of ErrorHandler that
collects all errors except those
I think the following conveys enough information in a single line:
"""Filters out errors that don't apply to Chromium Javascript code."""

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:46: import closure_linter.errors
Move the import at function level, otherwise this is inefficient. e.g. around
line 17.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:55: import closure_linter.checker
Keep all the imports together around line 17

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:61: join('chrome', 'browser', 'resources',
'ntp4'),
Note that you are already in chrome/browser/resources. So this function
shouldn't not receive any file outside this directory anyway. Do you need to
filter files in other subdirectories?

You can test for yourself:
1. touch a css in base or anywhere else
2. touch a css in chrome/browser/resources/ntp4
3. add both to a change
4. run the presubmit check locally. The css in base shouldn't be given to this
presubmit check. If it is, it's a bug.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:116:
'http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript'
you can split the line on two lines;
'http://www.chromium.org/developers/web-development-style-guide'
'#TOC-JavaScript'

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:122: def _CommonChecks(input_api,
output_api):
optional: This is probably overweight. I'd just call return it directly from
both function, e.g.
def CheckChangeOnUpload(input_api, output_api):
  return _CheckJavaScriptStyle(input_api, output_api)

you'd save 10 lines. :)

[Tyler Breisacher (Chromium), 2012-02-08 18:43:41]

https://chromiumcodereview.appspot.com/9288045/diff/23002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23002/PRESUBMIT.py#newcode9
PRESUBMIT.py:9: """
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> Let's remove changes to this file from this CL. I'll do it separately.

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:14:
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> if you remove www., it fits 80 cols. :)

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:20: """Implementation of ErrorHandler that
collects all errors except those
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> I think the following conveys enough information in a single line:
> """Filters out errors that don't apply to Chromium Javascript code."""

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:46: import closure_linter.errors
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> Move the import at function level, otherwise this is inefficient. e.g. around
> line 17.

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:55: import closure_linter.checker
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> Keep all the imports together around line 17

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:61: join('chrome', 'browser', 'resources',
'ntp4'),
True, but LocalPath() is relative to src/ so it still includes the
"chrome/browser/resources" part. Right now, we only want to run this check on
folders where the OWNERS have agreed to it, (and we've already gone through and
cleaned everything up so that  it passes the checks). Right now, that includes
only options2/ and ntp4/, not all of resources/.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:116:
'http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript'
On 2012/02/08 14:30:39, Marc-Antoine Ruel wrote:
> you can split the line on two lines;
> 'http://www.chromium.org/developers/web-development-style-guide'
> '#TOC-JavaScript'

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:122: def _CommonChecks(input_api,
output_api):
I was thinking Dan's CSS checking code would get merged into this file (or vice
versa if his lands first) but I guess we should just cross that bridge when we
come to it. Done.

[M-A Ruel, 2012-02-08 18:54:53]

lgtm

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23002/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:61: join('chrome', 'browser', 'resources',
'ntp4'),
On 2012/02/08 18:43:41, Tyler Breisacher wrote:
> True, but LocalPath() is relative to src/ so it still includes the
> "chrome/browser/resources" part. Right now, we only want to run this check on
> folders where the OWNERS have agreed to it, (and we've already gone through
and
> cleaned everything up so that  it passes the checks). Right now, that includes
> only options2/ and ntp4/, not all of resources/.

Oh that's annoying that the local path isn't rebased correctly, I didn't
realize.

https://chromiumcodereview.appspot.com/9288045/diff/17004/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/17004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:66: return
input_api.re.search('\.(js|html?)$', affected_file.LocalPath())
return affected_file.LocalPath().endswith(('.js', '.html'))
would be marginally faster. Note the double ().

[Dan Beam, 2012-02-08 19:46:54]

lgtm w/nits (correct me if any are against some style I don't know about,
maruel@)

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:19: import closure_linter.errors
nit: It'd save you one line and some typing of closure_linter. if you say

  from closure_linter import checker, errors
  from closure_linter.common import errorhandler

But I really don't care that much, so it's up to you.

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:25: self._errors = []
nit: Is there something in closure that makes you use _errors?  Otherwise, we
tend to use _ at the end (errors_) at least in C++/JavaScript, not at the front.
 I don't think I've seen this convention used in Python though, so wondering why
you need it at all?

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:96: errorMsg = '  line %d: E%04d: %s\n%s'
% (
nit: error_msg

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:112: '
http://www.chromium.org/developers/web-development-style-guide'
nit: spaces at the end instead of front, similar to operators

[M-A Ruel, 2012-02-08 19:57:24]

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:25: self._errors = []
On 2012/02/08 19:46:54, Dan Beam wrote:
> nit: Is there something in closure that makes you use _errors?  Otherwise, we
> tend to use _ at the end (errors_) at least in C++/JavaScript, not at the
front.
>  I don't think I've seen this convention used in Python though, so wondering
why
> you need it at all?

You didn't see it because I never use protected variables? The VM enforces it
for "private" members, namely __foo. self._errors is the right format for
python.

[Tyler Breisacher (Chromium), 2012-02-08 20:03:55]

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
File chrome/browser/resources/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:19: import closure_linter.errors
On 2012/02/08 19:46:54, Dan Beam wrote:
> nit: It'd save you one line and some typing of closure_linter. if you say
> 
>   from closure_linter import checker, errors
>   from closure_linter.common import errorhandler
> 
> But I really don't care that much, so it's up to you.

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:25: self._errors = []
Looks like this is right:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming...
though I agree it's strange we do _this in Python and this_ in C++.

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:96: errorMsg = '  line %d: E%04d: %s\n%s'
% (
On 2012/02/08 19:46:54, Dan Beam wrote:
> nit: error_msg

Done.

https://chromiumcodereview.appspot.com/9288045/diff/23004/chrome/browser/reso...
chrome/browser/resources/PRESUBMIT.py:112: '
http://www.chromium.org/developers/web-development-style-guide'
On 2012/02/08 19:46:54, Dan Beam wrote:
> nit: spaces at the end instead of front, similar to operators

Done.

[Tyler Breisacher (Chromium), 2012-02-08 20:18:37]

From http://code.google.com/p/chromium/issues/detail?id=80149#c7 it sounds like
const won't be optimized any *more* than var, but it's unlikely to slow us down
significantly either. I think const can be very helpful for the people reading
and writing code to convey intentions so it's worth allowing it IMO. So I'm
taking out the const check.

maruel: I'm still not sure of the best way to add closure_linter to the tree.
Should I add a DEPS entry or just download it and commit it directly?

[M-A Ruel, 2012-02-08 20:27:42]

On 2012/02/08 20:18:37, Tyler Breisacher wrote:
> From http://code.google.com/p/chromium/issues/detail?id=80149#c7 it sounds
like
> const won't be optimized any *more* than var, but it's unlikely to slow us
down
> significantly either. I think const can be very helpful for the people reading
> and writing code to convey intentions so it's worth allowing it IMO. So I'm
> taking out the const check.
> 
> maruel: I'm still not sure of the best way to add closure_linter to the tree.
> Should I add a DEPS entry or just download it and commit it directly?

How large?

[Tyler Breisacher (Chromium), 2012-02-08 21:34:41]

739.1 KB

[Dan Beam, 2012-02-08 22:00:00]

> The VM enforces it for "private" members, namely __foo. self._errors is the
right format for python.

Cool, good to know! :)

[M-A Ruel, 2012-02-09 00:35:11]

On 2012/02/08 21:34:41, Tyler Breisacher wrote:
> 739.1 KB

With or without test and doc?

What's the license?

You'll need to add it with the CL otherwise the presubmit check will be broken.

[Tyler Breisacher (Chromium), 2012-02-09 00:39:58]

On 2012/02/09 00:35:11, Marc-Antoine Ruel wrote:
> On 2012/02/08 21:34:41, Tyler Breisacher wrote:
> > 739.1 KB
> 
> With or without test and doc?
> 
> What's the license?
> 
> You'll need to add it with the CL otherwise the presubmit check will be
broken.

That's the size of a full checkout from trunk, including license and docs (and
the .svn dirs). License is Apache 2.0
(http://code.google.com/p/closure-linter/source/browse/trunk/LICENSE)

[M-A Ruel, 2012-02-09 01:34:39]

On 2012/02/09 00:39:58, Tyler Breisacher wrote:
> On 2012/02/09 00:35:11, Marc-Antoine Ruel wrote:
> > On 2012/02/08 21:34:41, Tyler Breisacher wrote:
> > > 739.1 KB
> > 
> > With or without test and doc?
> > 
> > What's the license?
> > 
> > You'll need to add it with the CL otherwise the presubmit check will be
> broken.
> 
> That's the size of a full checkout from trunk, including license and docs (and
> the .svn dirs). License is Apache 2.0
> (http://code.google.com/p/closure-linter/source/browse/trunk/LICENSE)

Ok, skip the directory testdata, add a README.chromium and add it to
third_party. Look at other third parties for examples. Note the revision you
fetched in the README.chromium file.

[Tyler Breisacher (Chromium), 2012-02-09 01:58:31]

jhawkins: Need an lg on this from a resources/ OWNER. (Probably want to land
https://chromiumcodereview.appspot.com/9316086/ first or at least at about the
same time, so let me know if that lgty as well :)

[James Hawkins, 2012-02-09 02:02:18]

On 2012/02/09 01:58:31, Tyler Breisacher wrote:
> jhawkins: Need an lg on this from a resources/ OWNER. (Probably want to land
> https://chromiumcodereview.appspot.com/9316086/ first or at least at about the
> same time, so let me know if that lgty as well :)

LGTM

[Tyler Breisacher (Chromium), 2012-02-14 19:14:15]

> Ok, skip the directory testdata, add a README.chromium and add it to
> third_party. Look at other third parties for examples. Note the revision you
> fetched in the README.chromium file.

Do we also need to do something to make sure that third_party/closure_linter is
added to the PATH or PYTHONPATH variable?

[Tyler Breisacher (Chromium), 2012-02-15 00:14:10]

Nevermind, just saw what Dan is doing with sys.path.insert in the other CL.
Looks like I should do something similar to that.

[Tyler Breisacher (Chromium), 2012-02-15 19:37:43]

On 2012/02/15 00:14:10, Tyler Breisacher wrote:
> Nevermind, just saw what Dan is doing with sys.path.insert in the other CL.
> Looks like I should do something similar to that.

I've rebased this to trunk so that it fits in with dbeam's CSS checker. The only
significant new code is where I'm changing sys.path so that we can import
closure_linter from third_party, in js_checker.py

[Dan Beam, 2012-02-15 19:42:47]

lgtm (because looks awesome to me [latm] doesn't trigger the magical green box)

[M-A Ruel, 2012-02-15 19:51:05]

lgtm

https://chromiumcodereview.appspot.com/9288045/diff/20004/chrome/browser/reso...
File chrome/browser/resources/web_dev_style/js_checker.py (right):

https://chromiumcodereview.appspot.com/9288045/diff/20004/chrome/browser/reso...
chrome/browser/resources/web_dev_style/js_checker.py:115: 
remove empty line.

[Tyler Breisacher (Chromium), 2012-02-15 22:42:59]

Sorry, one more question. Closure-linter depends on python-gflags (I didn't
notice because I had used easy_install before, so I had gflags in dist-packages)
so I guess we either need everyone to install python-gflags (via easy_install or
something like that) or we need to also add python-gflags to our source tree.
It's at http://code.google.com/p/python-gflags/ and weighs in at 278KB including
tests. So I would guess adding it to the source tree makes the most sense?

[Marc-Antoine Ruel (Google), 2012-02-15 22:47:56]

On 2012/02/15 22:42:59, Tyler Breisacher wrote:
> Sorry, one more question. Closure-linter depends on python-gflags (I didn't
> notice because I had used easy_install before, so I had gflags in
dist-packages)
> so I guess we either need everyone to install python-gflags (via easy_install
or
> something like that) or we need to also add python-gflags to our source tree.
> It's at http://code.google.com/p/python-gflags/ and weighs in at 278KB
including
> tests. So I would guess adding it to the source tree makes the most sense?

Don't expect people to have any specific python package.

Commit it without the tests.