PRESUBMIT check for JavaScript style errors

chrome/browser/resources/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.
+
+"""Presubmit script for Chromium HTML/CSS/JS resources.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into gcl.
+"""
+
+
+def _CheckJavaScriptStyle(input_api, output_api):
+  """Check for violations of the Chromium JavaScript style guide. See
+     http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript

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

if you remove www., it fits 80 cols. :)

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

Done.

+  """
+
+  import closure_linter.common.errorhandler
+
+  class ErrorHandlerImpl(closure_linter.common.errorhandler.ErrorHandler):
+    """Implementation of ErrorHandler that collects all errors except those

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

I think the following conveys enough information in a single line:
"""Filters out errors that don't apply to Chromium Javascript code."""

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

Done.

+       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

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

Move the import at function level, otherwise this is inefficient. e.g. around
line 17.

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

Done.

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

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

Keep all the imports together around line 17

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

Done.

+
+  # 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'),

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

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.

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

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

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

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

+      join('chrome', 'browser', 'resources', 'options2'),
+  ]
+
+  def in_checked_folder(affected_file):
+    return any(affected_file.LocalPath().startswith(cf)
+        for cf in checked_folders)
+
+  def js_or_html(affected_file):
+    return input_api.re.search('\.(js|html?)$', affected_file.LocalPath())
+
+  def file_filter(affected_file):
+    return js_or_html(affected_file) and in_checked_folder(affected_file)
+
+  results = []
+
+  for f in input_api.change.AffectedFiles(file_filter=file_filter):
+    error_lines = []
+
+    # check for getElementById()
+    for i, line in enumerate(f.NewContents(), start=1):
+      if 'getElementById' in line:
+        error_lines.append('  line %d: %s\n%s' % (
+            i,
+            "Use $('id') instead of document.getElementById('id')",
+            line))
+
+      if input_api.re.search(r'\bconst\b', line):
+        error_lines.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(join(input_api.change.RepositoryRoot(), 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)
+      error_lines.append(errorMsg)
+
+    if error_lines:
+      error_lines = [
+          'Found JavaScript style violations in %s:' %
+          f.LocalPath()] + error_lines
+      results.append(output_api.PresubmitError('\n'.join(error_lines)))
+
+  if results:
+    results.append(output_api.PresubmitNotifyResult(
+        'See the JavaScript style guide at '
+        'http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript'

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

you can split the line on two lines;
'http://www.chromium.org/developers/web-development-style-guide'
'#TOC-JavaScript'

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

Done.

+        ' and if you have any feedback about the JavaScript PRESUBMIT check,'
+        ' contact tbreisacher@chromium.org'))
+
+  return results
+
+def _CommonChecks(input_api, output_api):

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

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]

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.

+  """Checks common to both upload and commit."""
+  results = []
+  results.extend(_CheckJavaScriptStyle(input_api, output_api))
+  return results
+
+def CheckChangeOnUpload(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  return results
+
+def CheckChangeOnCommit(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  return results
+