PRESUBMIT check for JavaScript style errors

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index aa79141a90fa5217e6f29f1b7646c0744a21ffd5..b2aac3102c0e072b551699349201bad16d670def 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -9,9 +9,12 @@ for more details about the presubmit API built into gcl.
 """
 
 
+import closure_linter.checker
+import closure_linter.common.errorhandler
+import closure_linter.errors
+import os.path
 import re

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

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. 

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

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?

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

no. :)

 
-
 _EXCLUDED_PATHS = (
     r"^breakpad[\\\/].*",
     r"^native_client_sdk[\\\/].*",
@@ -29,7 +32,105 @@ _TEST_ONLY_WARNING = (
     'not perfect.  The commit queue will not block on this warning.\n'
     'Email joi@chromium.org if you have questions.')
 

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

2 vertical lines between file level symbols please.

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

Done.

+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 = []

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

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.

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

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

+
+  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 not self._errors.empty

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

What's list.empty?

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

*facepalm*. Thank you! Done.

+
+  def _valid(self, error):
+    '''Check whether an error is valid. Most errors are valid, with a few

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

All the rest of the file use """for docstrings""".

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

Done.

+       exceptions which are listed here.
+    '''
+    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
+    ]
+
+def _CheckJavaScriptStyle(input_api, output_api):
+  """Check for JavaScript style violations."""
 
+  # Only check the following folders. OWNERS of folders containing JavaScript
+  # code can opt-in to this check by adding the folder here.
+  checked_folders = [
+      os.path.join('chrome', 'browser', 'resources', 'ntp4'),
+      os.path.join('chrome', 'browser', 'resources', 'options2'),
+  ]
+
+  def inCheckedFolder(affected_file):
+    return any(affected_file.LocalPath().startswith(cf)
+        for cf in checked_folders)
+
+  def jsOrHtml(affected_file):
+    return re.search('\.(js|html?)$', affected_file.LocalPath())

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

input_api.re

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

Done.

+
+  def fileFilter(affected_file):
+    return jsOrHtml(affected_file) and inCheckedFolder(affected_file)
+
+  results = []
+
+  for f in input_api.change.AffectedFiles(file_filter=fileFilter):
+    errorLines = []
+
+    # check for getElementById()
+    for i, line in enumerate(f.NewContents()):

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

Off by one error! The first line is line 1, not line 0. (Done.)

+      if 'getElementById' in line:
+        errorLines.append('  line %d: %s\n%s' % (
+            i,
+            'Use $() instead of document.getElementById()',
+            line))
+
+      const_re = re.compile(r'\bconst\b')
+      if const_re.search(line):
+        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,
+          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'
+        ' and contact tbreisacher@chromium.org for feedback on this'
+        ' PRESUBMIT check.'))
+
+  return results
 
 def _CheckNoInterfacesInBase(input_api, output_api):
   """Checks to make sure no files in libbase.a have |@interface|."""
@@ -214,6 +315,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))