DevTools: use git cl format --js for PRESUBMIT check

third_party/WebKit/Source/devtools/PRESUBMIT.py

Index: third_party/WebKit/Source/devtools/PRESUBMIT.py
diff --git a/third_party/WebKit/Source/devtools/PRESUBMIT.py b/third_party/WebKit/Source/devtools/PRESUBMIT.py
index 80789396046e679fc7a5d6db25daf8ba9d69a629..2bd914d80930ff75ebe6f3a8eff3082e9295df54 100644
--- a/third_party/WebKit/Source/devtools/PRESUBMIT.py
+++ b/third_party/WebKit/Source/devtools/PRESUBMIT.py
@@ -43,55 +43,18 @@ def _CheckNodeAndNPMModules(input_api, output_api):
         [input_api.python_executable, node_script_path], stdout=input_api.subprocess.PIPE, stderr=input_api.subprocess.STDOUT)
     out, _ = process.communicate()
     if process.returncode != 0:
-        return CheckOutput([output_api.PresubmitError(out)], has_errors=True)
-    return CheckOutput([output_api.PresubmitNotifyResult(out)], has_errors=False)
+        return [output_api.PresubmitError(out)]
+    return [output_api.PresubmitNotifyResult(out)]
 
 
 def _FormatDevtools(input_api, output_api):
-    affected_files = _getAffectedJSFiles(input_api)
-    if len(affected_files) == 0:
-        return CheckOutput([], has_errors=False)
-    original_sys_path = sys.path
-    try:
-        sys.path = sys.path + [input_api.os_path.join(input_api.PresubmitLocalPath(), "scripts")]
-        import install_node_deps
-    finally:
-        sys.path = original_sys_path
-
-    node_path, _ = install_node_deps.resolve_node_paths()
-    format_path = input_api.os_path.join(input_api.PresubmitLocalPath(), "scripts", "format.js")
-    glob_arg = "--glob=" + ",".join(affected_files)
-    check_formatting_process = _inputPopen(input_api, args=[node_path, format_path] + [glob_arg, "--output-replacements-xml"])
-    check_formatting_out, _ = check_formatting_process.communicate()
-    if check_formatting_process.returncode != 0:
-        return CheckOutput([output_api.PresubmitError(check_formatting_out)], has_errors=True)
-    if "</replacement>" not in check_formatting_out:
+    check_formatting_process = input_api.subprocess.Popen(
+        args=['git', 'cl', 'format', '--js', '--dry-run'], stdout=input_api.subprocess.PIPE, stderr=input_api.subprocess.STDOUT)
+    check_formatting_process.communicate()
+    if check_formatting_process.returncode == 0:
         return CheckOutput([output_api.PresubmitNotifyResult("CL is properly formatted")], has_errors=False)
-
-    format_args = [node_path, format_path] + [glob_arg]
-    format_process = _inputPopen(input_api, format_args)
-    format_process_out, _ = format_process.communicate()
-
-    # Use eslint to autofix the braces

[dgozman, 2017/02/27 18:36:28]

That's unfortunate. Can we print out the command line to fix things?

[chenwilliam, 2017/02/28 20:28:38]

I can print out "npm run fix-braces" if they get an error. It does take ~8-10
sec now because it's running it over all of front_end (not just changed files)

-    eslint_path = input_api.os_path.join(input_api.PresubmitLocalPath(), "node_modules", ".bin", "eslint")
-    eslint_process = _inputPopen(
-        input_api,
-        [node_path, eslint_path, '--no-eslintrc', '--fix', '--env=es6', '--rule={"curly": [2, "multi-or-nest", "consistent"]}'] +
-        affected_files)
-    eslint_process.communicate()
-
-    # Need to run clang-format again to align the braces
-    reformat_process = _inputPopen(input_api, format_args)
-    reformat_process.communicate()
-
     return CheckOutput(
-        [
-            output_api.PresubmitError("ERROR: Found formatting violations.\n"
-                                      "Ran clang-format on files changed in CL\n"
-                                      "Use git status to check the formatting changes"),
-            output_api.PresubmitError(format_process_out)
-        ],
-        has_errors=True)
+        [output_api.PresubmitError("ERROR: Found formatting violations.\nPlease run 'git cl format --js'")], has_errors=True)

[dgozman, 2017/02/27 18:36:28]

The common script say something like "directory X requires formatting. Please
run git cl format X".

Also, can we just use standard script?

[chenwilliam, 2017/02/28 20:28:38]

I looked into the precanned check but I'd like to have our own check so:
1) we can tell people about "npm run fix-braces"
2) make it an error and not a warning (which is what the precanned check does).

 
 
 def _CheckDevtoolsStyle(input_api, output_api):
@@ -189,18 +152,13 @@ def _CheckCSSViolations(input_api, output_api):
 def CheckChangeOnUpload(input_api, output_api):
     results = []
 
-    (node_results, has_errors) = _CheckNodeAndNPMModules(input_api, output_api)
-    results.extend(node_results)
-    if has_errors:
-        results.append(output_api.PresubmitError("ERROR: Bailed out early because error using node.js/npm"))
-        return results
-
     (format_results, has_errors) = _FormatDevtools(input_api, output_api)
     results.extend(format_results)
     if has_errors:
         results.append(output_api.PresubmitError("ERROR: Bailed out early because formatting errors were found"))

[dgozman, 2017/02/27 18:36:28]

Do we need to bailout now? Compilation is fast.

[chenwilliam, 2017/02/28 20:28:38]

Removed bailout.

         return results
 
+    results.extend(_CheckNodeAndNPMModules(input_api, output_api))
     results.extend(_CheckDevtoolsStyle(input_api, output_api))
     results.extend(_CompileDevtoolsFrontend(input_api, output_api))
     results.extend(_CheckConvertSVGToPNGHashes(input_api, output_api))
@@ -221,19 +179,3 @@ def _getAffectedFrontEndFiles(input_api):
         file_name for file_name in local_paths if devtools_front_end in file_name and file_name.endswith(".js")
     ]
     return [input_api.os_path.relpath(file_name, devtools_root) for file_name in affected_front_end_files]
-
-
-def _getAffectedJSFiles(input_api):
-    local_paths = [f.AbsoluteLocalPath() for f in input_api.AffectedFiles() if f.Action() != "D"]
-    devtools_root = input_api.PresubmitLocalPath()
-    devtools_front_end = input_api.os_path.join(devtools_root, "front_end")
-    devtools_scripts = input_api.os_path.join(devtools_root, "scripts")
-    affected_js_files = [
-        file_name for file_name in local_paths
-        if (devtools_front_end in file_name or devtools_scripts in file_name) and file_name.endswith(".js")
-    ]
-    return [input_api.os_path.relpath(file_name, devtools_root) for file_name in affected_js_files]
-
-
-def _inputPopen(input_api, args):
-    return input_api.subprocess.Popen(args, stdout=input_api.subprocess.PIPE, stderr=input_api.subprocess.STDOUT)