PRESUBMIT check for JavaScript style errors

PRESUBMIT.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 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[\\\/].*",
     r"^net[\\\/]tools[\\\/]spdyshark[\\\/].*",
     r"^skia[\\\/].*",
     r"^v8[\\\/].*",
     r".*MakeFile$",
 )
 
 
 _TEST_ONLY_WARNING = (
     'You might be calling functions intended only for testing from\n'
     'production code.  It is OK to ignore this warning if you know what\n'
     'you are doing, as the heuristics used to detect the situation are\n'
     '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|."""
   pattern = input_api.re.compile(r'^\s*@interface', input_api.re.MULTILINE)
   files = []
   for f in input_api.AffectedSourceFiles(input_api.FilterSourceFile):
     if (f.LocalPath().startswith('base/') and
         not f.LocalPath().endswith('_unittest.mm')):
       contents = input_api.ReadFile(f)
       if pattern.search(contents):
@@ skipping 164 matching lines @@
   if not problems:
     return []
   return [output_api.PresubmitPromptWarning('The old callback system is '
       'deprecated. If possible, use base::Bind and base::Callback instead.\n' +
       '\n'.join(problems))]
 
 
 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))
   results.extend(_CheckAuthorizedAuthor(input_api, output_api))
   results.extend(
     _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api))
   results.extend(_CheckNoIOStreamInHeaders(input_api, output_api))
   results.extend(_CheckNoNewWStrings(input_api, output_api))
   results.extend(_CheckNoDEPSGIT(input_api, output_api))
   results.extend(_CheckNoFRIEND_TEST(input_api, output_api))
@@ skipping 109 matching lines @@
 def GetPreferredTrySlaves(project, change):
   only_objc_files = all(
       f.LocalPath().endswith(('.mm', '.m')) for f in change.AffectedFiles())
   if only_objc_files:
     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()):
     preferred.append('linux_chromeos')
   return preferred