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://chromium.org/developers/web-development-style-guide#TOC-JavaScript
+  """
+
+  import closure_linter.checker
+  import closure_linter.common.errorhandler
+  import closure_linter.errors

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

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.

[Tyler Breisacher (Chromium), 2012/02/08 20:03:56]

Done.

+
+  class ErrorHandlerImpl(closure_linter.common.errorhandler.ErrorHandler):
+    """Filters out errors that don't apply to Chromium JavaScript code."""
+
+    def __init__(self):
+      self._errors = []

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

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?

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

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:56]

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

+
+    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.
+      """
+
+      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,
+      ]
+
+  # 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 in_checked_folder(affected_file):
+    return any(affected_file.LocalPath().startswith(cf)
+        for cf in checked_folders)
+
+  def js_or_html(affected_file):
+    return affected_file.LocalPath().endswith(('.js', '.html'))
+
+  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' % (

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

nit: error_msg

[Tyler Breisacher (Chromium), 2012/02/08 20:03:56]

Done.

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

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

nit: spaces at the end instead of front, similar to operators

[Tyler Breisacher (Chromium), 2012/02/08 20:03:56]

Done.

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