WebUI: Hook up ESLint to web_dev_style PRESUBMIT.

tools/web_dev_style/js_checker.py

Index: tools/web_dev_style/js_checker.py
diff --git a/tools/web_dev_style/js_checker.py b/tools/web_dev_style/js_checker.py
index a42a1a22aeef70d5e538bde352d2f0fac7aaf38a..15acfcf3dc8f1c87311399e84cde4d1f039fd86b 100644
--- a/tools/web_dev_style/js_checker.py
+++ b/tools/web_dev_style/js_checker.py
@@ -81,6 +81,36 @@ class JSChecker(object):
         r"var (?!g_\w+)(_?[a-z][a-zA-Z]*[_$][\w_$]*)(?<! \$)",
         "Please use var namesLikeThis <https://goo.gl/eQiXVW>")
 

[Dan Beam, 2017/05/24 02:13:04]

\n\n between file-level globals

[dpapad, 2017/05/24 22:43:42]

This file does not seem to follow that rule (all methods are separated by a
single \n). How about we change this file to comply with \n\n rule as a whole in
a follow up CL?

+  def RunEsLintChecks(self, affected_js_files):

[Dan Beam, 2017/05/24 02:13:04]

arguable nit: maybe insert this method alphabetically (looks like others are
sorted with the exception of RegexCheck, which seems to be shared)

[dpapad, 2017/05/24 22:43:42]

Done.

+    """Runs lint checks using ESLint. The ESLint rules being applied are defined
+       in the .eslintrc.js configuration file.
+    """
+    # Import ESLint.
+    import os
+    _HERE_PATH = os.path.dirname(os.path.realpath(__file__))
+    _SRC_PATH = os.path.normpath(os.path.join(_HERE_PATH, '..', '..'))
+    import sys
+    sys.path.append(os.path.join(_SRC_PATH, 'third_party', 'node'))

[Dan Beam, 2017/05/24 02:13:04]

you should undo the effects of this sys.path munging with try: finally:

[dpapad, 2017/05/24 22:43:42]

Done. Note, I only wrapped the part that imports in a try finally, instead of
wrapping the entire function.

+    import node
+    import node_modules
+
+    # Extract paths to be passed to ESLint.
+    affected_js_files_paths = []
+    presubmit_path = self.input_api.PresubmitLocalPath()
+    for f in affected_js_files:
+      affected_js_files_paths.append(
+          os.path.relpath(f.AbsoluteLocalPath(), presubmit_path))

[Dan Beam, 2017/05/24 02:13:04]

hmmmm, is there a reason you have to ask for the /Absolute/ path then make it
relative?  can we use just .LocalPath() instead?

[dpapad, 2017/05/24 22:43:42]

I did not find a way to achieve the correct path with LocalPath(). Example:

presbumit_path:
/usr/local/home/myself/workspace/chromium1/src/chrome/browser/resources
f.LocalPath(): chrome/browser/resources/settings/direction_delegate.js
f.AbsolutePath():
/usr/local/home/myself/workspace/chromium1/src/chrome/browser/resources/settings/direction_delegate.js


os.path.relpath(f.AbsoluteLocalPath(), presubmit_path)) -->
settings/direction_delegate.js
os.path.relpath(f.LocalPath(), presubmit_path)) -->
chrome/browser/resources/settings/direction_delegate.js

The path that ESLint accepts must be either
1) relative to the CWD (which is presubmit_path) or
2) an absolute path.

I chose 1, to pass the shortest possible valid path to ESLint.
Do you think prefer 2? As follows (which would also work)

affected_js_files_paths.append(f.AbsolutePath()) 

+
+    output = node.RunNode([
+        node_modules.PathToEsLint(),
+        '--color',
+        '--ignore-pattern \'!.eslintrc.js\'',
+        ' '.join(affected_js_files_paths)])
+
+    if len(output) > 0:

[Dan Beam, 2017/05/24 02:13:04]

empty arrays are falsy in python, so you can just do

  if output:

or better yet:

  return [self.output_api.PresubmitError(output)] if output else []

[dpapad, 2017/05/24 22:43:42]

Done.

+      return [self.output_api.PresubmitError(output)]
+    return []
+
   def _GetErrorHighlight(self, start, length):
     """Takes a start position and a length, and produces a row of '^'s to
        highlight the corresponding part of a string.
@@ -97,6 +127,10 @@ class JSChecker(object):
                                                   include_deletes=False)
     affected_js_files = filter(lambda f: f.LocalPath().endswith('.js'),
                                affected_files)
+
+    if len(affected_js_files) > 0:

[Dan Beam, 2017/05/24 02:13:05]

same nit, re: if len(x) > 0 vs just if x:

[dpapad, 2017/05/24 22:43:42]

Done.

+      results += self.RunEsLintChecks(affected_js_files)
+
     for f in affected_js_files:
       error_lines = []