[presubmit] check for long lines in HTML files

chrome/browser/resources/PRESUBMIT.py

 # Copyright 2014 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 files in chrome/browser/resources.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 """
 
@@ skipping 73 matching lines @@
 
   html_element_re = r'<(.*?)(^|\s+)metric\s*=\s*"%s"(.*?)>' % (metric_name)
   type_re = (r'datatype\s*=\s*"boolean"|type\s*=\s*"checkbox"|'
       'type\s*=\s*"radio".*?value\s*=\s*("true"|"false")')
 
   match = input_api.re.search(html_element_re, new_content, input_api.re.DOTALL)
   return (match and
       any(input_api.re.search(type_re, match.group(i)) for i in (1, 3)))
 
 
+def CheckHtml(input_api, output_api):
+  return input_api.canned_checks.CheckLongLines(input_api, output_api, 80)

[tommycli, 2017/03/06 22:06:15]

Read 2nd comment first...

seems a bit redundant to repeat the 80 col limit same as in the below place...
Maybe that's another sign it belongs there?

+
+
 def RunVulcanizeTests(input_api, output_api):
   presubmit_path = input_api.PresubmitLocalPath()
   tests = [input_api.os_path.join(presubmit_path, 'vulcanize_gn_test.py')]
   return input_api.canned_checks.RunUnitTests(input_api, output_api, tests)
 
 
 def _CheckChangeOnUploadOrCommit(input_api, output_api):
   results = CheckUserActionUpdate(input_api, output_api, ACTION_XML_PATH)
   affected = input_api.AffectedFiles()
+  if any(f for f in affected if f.LocalPath().endswith('.html')):
+    results += CheckHtml(input_api, output_api)

[tommycli, 2017/03/06 22:06:15]

I wonder why this isn't included already via:
https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py...
?

/ Does it make more sense to put it there?

[dschuyler, 2017/03/07 01:14:56]

Afaik, the canned checks provide functionality, but it's still the choice of
different areas of the code about whether to run it.

We could:
1. run all the canned checks (at that link) here, but that adds a collection of
checks.
2. Or we could change the broader code checker to always include HTML.
3. Or there's this option :) Running one canned test in a rather narrow set of
files.

Options 1 and 2 are more than I'm ready to sign up for (in case there are
issues). I'm looking to make progress on code health, without derailing my
primary focus. IMO, this is a balanced step forward.

   if any(f for f in affected if f.LocalPath().endswith('vulcanize_gn.py')):
     results += RunVulcanizeTests(input_api, output_api)
   return results
 
 
 def CheckChangeOnUpload(input_api, output_api):
   return _CheckChangeOnUploadOrCommit(input_api, output_api)
 
 
 def CheckChangeOnCommit(input_api, output_api):
   return _CheckChangeOnUploadOrCommit(input_api, output_api)
 
 
 def PostUploadHook(cl, change, output_api):
   return output_api.EnsureCQIncludeTrybotsAreAdded(
     cl,
     [
       'master.tryserver.chromium.linux:closure_compilation',
     ],
     'Automatically added optional Closure bots to run on CQ.')