Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907353005:
DevTools: Add lint_javascript.py to drive eslint (Closed)
|
Created:
4 years, 8 months ago by paulirish Modified:
4 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add lint_javascript.py to drive eslint
An install of eslint (and Node.js) is required.
npm install -g eslint
http://eslint.org/docs/user-guide/getting-started
BUG=605878
Committed: https://crrev.com/37e52b98e568d1e2dcbfed59e783816b69c59d83
Cr-Commit-Position: refs/heads/master@{#390567}
Patch Set 1 #Patch Set 2 : remove extra log #
Total comments: 8
Patch Set 3 : double quotes. copyright header #Patch Set 4 : move errors_found to be local" #
Total comments: 2
Patch Set 5 : closure of errors_found n all that #
Messages
Total messages: 23 (8 generated)
paulirish@chromium.org changed reviewers: + caseq@chromium.org
On 2016/04/22 at 21:33:52, paulirish wrote: > Why remove "/" from the directories on the ignore? Do they occur in subfolders as well?
On 2016/04/22 at 22:45:26, jonathan.garbee wrote: > On 2016/04/22 at 21:33:52, paulirish wrote: > > > > Why remove "/" from the directories on the ignore? Do they occur in subfolders as well? Good question. eslint only looks for .eslintignore in its CWD, which is not useful in this case, when a script (in a different CWD) is driving eslint. While we can force the location for eslintignore, it doesn't handle the relative paths correctly. Google `eslintignore not working` to see a bunch of threads of people upset at this behavior. Dropping the slashes is a decent workaround until the bug is fixed upstream.
On 2016/04/23 at 00:10:35, paulirish wrote: > On 2016/04/22 at 22:45:26, jonathan.garbee wrote: > > On 2016/04/22 at 21:33:52, paulirish wrote: > > > > > > > Why remove "/" from the directories on the ignore? Do they occur in subfolders as well? > > Good question. eslint only looks for .eslintignore in its CWD, which is not useful in this case, when a script (in a different CWD) is driving eslint. While we can force the location for eslintignore, it doesn't handle the relative paths correctly. Google `eslintignore not working` to see a bunch of threads of people upset at this behavior. Dropping the slashes is a decent workaround until the bug is fixed upstream. Awesome. LGTM FWIW.
https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/lint_javascript.py (right): https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:2: # Copyright (c) 2012 Google Inc. All rights reserved. Let's bring the 3-line chromium authors copyright? https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:39: if sys.argv[1] == '--help': Let's settle on one quote style? Either is good per code style, but I guess we could make it double quotes for consistency with JS. https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:95: errors_found = False move into js_lint? https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:122: print eslint_proc_out move out of conditional?
paulirish@chromium.org changed reviewers: + jonathan.garbee@chromium.org
updated. ptal https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/lint_javascript.py (right): https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:2: # Copyright (c) 2012 Google Inc. All rights reserved. On 2016/04/26 at 01:39:44, caseq wrote: > Let's bring the 3-line chromium authors copyright? done https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:39: if sys.argv[1] == '--help': On 2016/04/26 at 01:39:44, caseq wrote: > Let's settle on one quote style? Either is good per code style, but I guess we could make it double quotes for consistency with JS. double. you got it. https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:95: errors_found = False On 2016/04/26 at 01:39:44, caseq wrote: > move into js_lint? Seems good. done. https://codereview.chromium.org/1907353005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:122: print eslint_proc_out On 2016/04/26 at 01:39:44, caseq wrote: > move out of conditional? done
lgtm https://codereview.chromium.org/1907353005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/lint_javascript.py (right): https://codereview.chromium.org/1907353005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:105: if errors_found: nit: I'd leave this one on the top level, I just thought we want the errors_found to be declared within js_lint() so that references within the function use local var, not one from closure.
https://codereview.chromium.org/1907353005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/lint_javascript.py (right): https://codereview.chromium.org/1907353005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/lint_javascript.py:105: if errors_found: On 2016/04/26 at 03:47:18, caseq wrote: > nit: I'd leave this one on the top level, I just thought we want the errors_found to be declared within js_lint() so that references within the function use local var, not one from closure. groovy. done.
The CQ bit was checked by paulirish@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jonathan.garbee@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/1907353005/#ps80001 (title: "closure of errors_found n all that")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907353005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907353005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
paulirish@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
+dgozman and pfeldman for OWNERS
lgtm
The CQ bit was checked by paulirish@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907353005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907353005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/37e52b98e568d1e2dcbfed59e783816b69c59d83 Cr-Commit-Position: refs/heads/master@{#390567}
Message was sent while issue was closed.
Description was changed from
==========
DevTools: Add lint_javascript.py to drive eslint
An install of eslint (and Node.js) is required.
npm install -g eslint
http://eslint.org/docs/user-guide/getting-started
BUG=605878
==========
to
==========
DevTools: Add lint_javascript.py to drive eslint
An install of eslint (and Node.js) is required.
npm install -g eslint
http://eslint.org/docs/user-guide/getting-started
BUG=605878
Committed: https://crrev.com/37e52b98e568d1e2dcbfed59e783816b69c59d83
Cr-Commit-Position: refs/heads/master@{#390567}
==========
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
