Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729403003:
[presubmit] check for long lines in HTML files (Closed)
Description[presubmit] check for long lines in HTML files
This CL checks for HTML files that have lines longer than 80 characters,
with the exception of <link ...> lines which are not limited. This will
apply to HTML files within the chrome/browser/resources directory. (It
could be applied elsewhere, but let's start with just this directory).
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2729403003
Cr-Commit-Position: refs/heads/master@{#455567}
Committed: https://chromium.googlesource.com/chromium/src/+/3ae1581ed4f4a256e32d5687638c74c718380aef
Patch Set 1 #
Total comments: 3
Messages
Total messages: 15 (9 generated)
Description was changed from ========== [presubmit] check for long lines in HTML files This CL checks for HTML files that have lines longer than 80 characters, with the exception of <link ...> lines which are not limited. This will apply to HTML files within the chrome/browser/resources directory. (It could be applied elsewhere, but let's start with just this directory). BUG=None ========== to ========== [presubmit] check for long lines in HTML files This CL checks for HTML files that have lines longer than 80 characters, with the exception of <link ...> lines which are not limited. This will apply to HTML files within the chrome/browser/resources directory. (It could be applied elsewhere, but let's start with just this directory). BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + tommycli@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
had some questions https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:95: return input_api.canned_checks.CheckLongLines(input_api, output_api, 80) 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? https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:108: results += CheckHtml(input_api, output_api) 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?
https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:108: results += CheckHtml(input_api, output_api) On 2017/03/06 22:06:15, tommycli wrote: > 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? 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.
On 2017/03/07 01:14:56, dschuyler wrote: > https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... > File chrome/browser/resources/PRESUBMIT.py (right): > > https://codereview.chromium.org/2729403003/diff/1/chrome/browser/resources/PR... > chrome/browser/resources/PRESUBMIT.py:108: results += CheckHtml(input_api, > output_api) > On 2017/03/06 22:06:15, tommycli wrote: > > 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? > > 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. Okay... I've considered what you said. Seems okay. lgtm
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1489011238110380, "parent_rev":
"fbc99249d5d02c81d0f42ffb9f6af28b1451f442", "commit_rev":
"3ae1581ed4f4a256e32d5687638c74c718380aef"}
Message was sent while issue was closed.
Description was changed from ========== [presubmit] check for long lines in HTML files This CL checks for HTML files that have lines longer than 80 characters, with the exception of <link ...> lines which are not limited. This will apply to HTML files within the chrome/browser/resources directory. (It could be applied elsewhere, but let's start with just this directory). BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [presubmit] check for long lines in HTML files This CL checks for HTML files that have lines longer than 80 characters, with the exception of <link ...> lines which are not limited. This will apply to HTML files within the chrome/browser/resources directory. (It could be applied elsewhere, but let's start with just this directory). BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2729403003 Cr-Commit-Position: refs/heads/master@{#455567} Committed: https://chromium.googlesource.com/chromium/src/+/3ae1581ed4f4a256e32d5687638c... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/3ae1581ed4f4a256e32d5687638c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
