PRESUBMIT check for JavaScript style errors

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index aa79141a90fa5217e6f29f1b7646c0744a21ffd5..a4b8d2832b3af727f44341b29703ced92cd5eb2f 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -8,10 +8,6 @@ See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
 
-
-import re
-
-
 _EXCLUDED_PATHS = (
     r"^breakpad[\\\/].*",
     r"^native_client_sdk[\\\/].*",
@@ -30,6 +26,114 @@ _TEST_ONLY_WARNING = (
     'Email joi@chromium.org if you have questions.')
 
 
+def _CheckJavaScriptStyle(input_api, output_api):
+  """Check for JavaScript style violations."""

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

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.

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

Done.

+
+  import closure_linter.common.errorhandler

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

Are these actually local imports or is it the same as doing it at the top of the
file?

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

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

+
+  class ErrorHandlerImpl(closure_linter.common.errorhandler.ErrorHandler):
+    """Implementation of ErrorHandler that collects all errors except those
+       that don't apply for Chromium JavaScript code.
+    """
+
+    def __init__(self):
+      self._errors = []
+
+    def HandleFile(self, filename, first_token):
+      self._filename = filename
+
+    def HandleError(self, error):
+      if (self._valid(error)):
+        error.filename = self._filename
+        self._errors.append(error)
+
+    def GetErrors(self):
+      return self._errors
+
+    def HasErrors(self):
+      return bool(self._errors)
+
+    def _valid(self, error):
+      """Check whether an error is valid. Most errors are valid, with a few
+         exceptions which are listed here.
+      """
+
+      import closure_linter.errors
+
+      return error.code not in [
+          closure_linter.errors.COMMA_AT_END_OF_LITERAL,
+          closure_linter.errors.JSDOC_ILLEGAL_QUESTION_WITH_PIPE,
+          closure_linter.errors.
+            JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER,
+      ]
+
+  import closure_linter.checker
+
+  # Only check the following folders. OWNERS of folders containing JavaScript
+  # code can opt-in to this check by adding the folder here.
+  join = input_api.os_path.join
+  checked_folders = [
+      join('chrome', 'browser', 'resources', 'ntp4'),
+      join('chrome', 'browser', 'resources', 'options2'),
+  ]
+
+  def inCheckedFolder(affected_file):

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

I thought we used in_checked_folder for local functions in python?

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

Right.

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

Done.

+    return any(affected_file.LocalPath().startswith(cf)

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

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...

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

Good point, I'm not sure what we can do about it either.

+        for cf in checked_folders)
+
+  def jsOrHtml(affected_file):
+    return input_api.re.search('\.(js|html?)$', affected_file.LocalPath())

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

Why are you checking HTML?

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

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.

+
+  def fileFilter(affected_file):
+    return jsOrHtml(affected_file) and inCheckedFolder(affected_file)
+
+  results = []
+
+  for f in input_api.change.AffectedFiles(file_filter=fileFilter):
+    errorLines = []

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

camelCase in JS, under_scores in Python and C++ for locals, as far as I have
even seen

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

Done.

+
+    # check for getElementById()
+    for i, line in enumerate(f.NewContents(), start=1):

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

Whay are you starting at 1?

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

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

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

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.

+      if 'getElementById' in line:

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

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...

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

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 ;)

+        errorLines.append('  line %d: %s\n%s' % (
+            i,
+            'Use $() instead of document.getElementById()',

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

nit: $('id') to show we're not using jQuery (though MooTools and Prototype still
do this...)

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

Done.

+            line))
+
+      if input_api.re.search(r'\bconst\b', line):

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

Doesn't closure have something for this?

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

You would think so, but no :(

+        errorLines.append('  line %d: %s\n%s' % (
+            i,
+            'Use |var| instead of |const|. See http://crbug.com/80149',
+            line))
+
+    # Use closure_linter to check for several different errors
+    error_handler = ErrorHandlerImpl()
+    checker = closure_linter.checker.JavaScriptStyleChecker(error_handler)
+    checker.Check(f.LocalPath())
+
+    for error in error_handler.GetErrors():
+      errorMsg = '  line %d: E%04d: %s\n%s' % (
+          error.token.line_number,
+          error.code,

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

Do you think the average user will care about this error code?

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

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.

+          error.message,
+          error.token.line)
+      errorLines.append(errorMsg)
+
+    if errorLines:
+      errorLines = [
+          'Found JavaScript style violations in %s:' %
+          f.LocalPath()] + errorLines
+      results.append(output_api.PresubmitError('\n'.join(errorLines)))
+
+  if results:
+    results.append(output_api.PresubmitNotifyResult(
+        'See the JavaScript style guide at '
+        'http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml'

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

This is not necessarily the guide we're following.  You should be linking to and
following the local most (Chromium) style guide first.

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

Done.

+        ' and contact tbreisacher@chromium.org for feedback on this'

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

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.

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

Done.

+        ' PRESUBMIT check.'))
+
+  return results
+
 
 def _CheckNoInterfacesInBase(input_api, output_api):
   """Checks to make sure no files in libbase.a have |@interface|."""
@@ -214,6 +318,7 @@ def _CheckNoNewOldCallback(input_api, output_api):
 def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
   results = []
+  results.extend(_CheckJavaScriptStyle(input_api, output_api))
   results.extend(input_api.canned_checks.PanProjectChecks(
       input_api, output_api, excluded_paths=_EXCLUDED_PATHS))
   results.extend(_CheckNoInterfacesInBase(input_api, output_api))
@@ -340,6 +445,7 @@ def GetPreferredTrySlaves(project, change):
     return ['mac_rel']
   preferred = ['win_rel', 'linux_rel', 'mac_rel']
   aura_re = '_aura[^/]*[.][^/]*'
-  if any(re.search(aura_re, f.LocalPath()) for f in change.AffectedFiles()):
+  if any(input_api.re.search(aura_re, f.LocalPath())

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

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@ ^ ?

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

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.

+      for f in change.AffectedFiles()):
     preferred.append('linux_chromeos')
   return preferred