Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872703003:
WebUI: Hook up ESLint to web_dev_style PRESUBMIT. (Closed)
DescriptionWebUI: Hook up ESLint to web_dev_style PRESUBMIT.
Using a trivial .eslintrc.js config for now, just so that the checks
can be enabled. More lint rules will be added in follow up CLs.
BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2872703003
Cr-Commit-Position: refs/heads/master@{#474720}
Committed: https://chromium.googlesource.com/chromium/src/+/ff82d51d2b29b40486761fb60a2521af5f73e3cb
Patch Set 1 : More #Patch Set 2 : Remove semicolon. #Patch Set 3 : Merge conflicts. #Patch Set 4 : Move to web_dev_style #Patch Set 5 : Update #Patch Set 6 : Fix case where no affected files exist. #Patch Set 7 : Fix path. #
Total comments: 12
Patch Set 8 : Addressing comments. #
Total comments: 9
Patch Set 9 : Address comments. #
Total comments: 2
Messages
Total messages: 38 (25 generated)
Description was changed from ========== WebUI: Add PRESUBMIT warningns for ESLint style violations. BUG=None ========== to ========== WebUI: Add PRESUBMIT warningns for ESLint style violations. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== WebUI: Add PRESUBMIT warningns for ESLint style violations. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add PRESUBMIT warningns for ESLint style violations. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WebUI: Add PRESUBMIT warningns for ESLint style violations. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add PRESUBMIT warnings for ESLint style violations. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
Description was changed from ========== WebUI: Add PRESUBMIT warnings for ESLint style violations. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Hook up ESLint to web_dev style PRESUBMIT. Using a dummy .eslintrc.js config for now, just so that the checks can be enabled. ESLint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WebUI: Hook up ESLint to web_dev style PRESUBMIT. Using a dummy .eslintrc.js config for now, just so that the checks can be enabled. ESLint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Hook up ESLint to web_dev style PRESUBMIT. Using a trivial .eslintrc.js config for now, just so that the checks can be enabled. More lint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dpapad@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...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org, dglazkov@chromium.org
@dbeam: Please review tools/web_dev_style, and .eslintrc.js @dglazkov: Please OWNERS file change.
On 2017/05/23 at 23:01:17, dpapad wrote: > @dbeam: Please review tools/web_dev_style, and .eslintrc.js > @dglazkov: Please OWNERS file change. I should also note that I am not planning to land that CL until after I fix the existing violations in the code (at https://codereview.chromium.org/2902033002). The overall plan is described in more detail at https://bugs.chromium.org/p/chromium/issues/detail?id=720034#c9.
Description was changed from ========== WebUI: Hook up ESLint to web_dev style PRESUBMIT. Using a trivial .eslintrc.js config for now, just so that the checks can be enabled. More lint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Hook up ESLint to web_dev_style PRESUBMIT. Using a trivial .eslintrc.js config for now, just so that the checks can be enabled. More lint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks pretty good to me! https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:83: \n\n between file-level globals https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:84: def RunEsLintChecks(self, affected_js_files): arguable nit: maybe insert this method alphabetically (looks like others are sorted with the exception of RegexCheck, which seems to be shared) https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:93: sys.path.append(os.path.join(_SRC_PATH, 'third_party', 'node')) you should undo the effects of this sys.path munging with try: finally: https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:102: os.path.relpath(f.AbsoluteLocalPath(), presubmit_path)) hmmmm, is there a reason you have to ask for the /Absolute/ path then make it relative? can we use just .LocalPath() instead? https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:110: if len(output) > 0: 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 [] https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:131: if len(affected_js_files) > 0: same nit, re: if len(x) > 0 vs just if x:
lgtm
https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:83: On 2017/05/24 at 02:13:04, Dan Beam wrote: > \n\n between file-level globals 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? https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:84: def RunEsLintChecks(self, affected_js_files): On 2017/05/24 at 02:13:04, Dan Beam wrote: > arguable nit: maybe insert this method alphabetically (looks like others are sorted with the exception of RegexCheck, which seems to be shared) Done. https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:93: sys.path.append(os.path.join(_SRC_PATH, 'third_party', 'node')) On 2017/05/24 at 02:13:04, Dan Beam wrote: > you should undo the effects of this sys.path munging with try: finally: Done. Note, I only wrapped the part that imports in a try finally, instead of wrapping the entire function. https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:102: os.path.relpath(f.AbsoluteLocalPath(), presubmit_path)) On 2017/05/24 at 02:13:04, Dan Beam wrote: > hmmmm, is there a reason you have to ask for the /Absolute/ path then make it relative? can we use just .LocalPath() instead? 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()) https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:110: if len(output) > 0: On 2017/05/24 at 02:13:04, Dan Beam wrote: > 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 [] Done. https://codereview.chromium.org/2872703003/diff/140001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:131: if len(affected_js_files) > 0: On 2017/05/24 at 02:13:05, Dan Beam wrote: > same nit, re: if len(x) > 0 vs just if x: Done.
https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:77: import os use self.input_api.os_path instead of `import os` https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:78: _HERE_PATH = os.path.dirname(os.path.realpath(__file__)) i'm not totalllly sure how __file__ works here, but can you use input_api.PresubmitLocalPath() instead maybe? __file__ doesn't exist when eval()'d
i guess these are all nits, so lgtm https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:82: old_path = sys.path[::] nit: [::] -> [:] https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:85: import node_modules import node, node_modules
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:77: import os On 2017/05/24 at 22:57:53, Dan Beam wrote: > use self.input_api.os_path instead of `import os` Done. https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:78: _HERE_PATH = os.path.dirname(os.path.realpath(__file__)) On 2017/05/24 at 22:57:53, Dan Beam wrote: > i'm not totalllly sure how __file__ works here, but can you use > > input_api.PresubmitLocalPath() instead maybe? > > __file__ doesn't exist when eval()'d input_api.PresubmitLocalPath() returns the location of the current PRESUBMIT.py file, not the location of js_checker.py file, which is what needed here. For example when modifying a file inside chrome/browser/resources/settings/, it would return chrome/browser/resources/ (because that's where the PRESUBMIT.py file resides). Is js_checker.py ever invoked with eval()? I have been using 'git cl presubmit' to test my changes locally and that line seems to always work as expected. https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:82: old_path = sys.path[::] On 2017/05/24 at 23:01:34, Dan Beam wrote: > nit: [::] -> [:] Done. https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:85: import node_modules On 2017/05/24 at 23:01:34, Dan Beam wrote: > import node, node_modules Done.
https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/160001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:78: _HERE_PATH = os.path.dirname(os.path.realpath(__file__)) On 2017/05/24 23:46:02, dpapad wrote: > On 2017/05/24 at 22:57:53, Dan Beam wrote: > > i'm not totalllly sure how __file__ works here, but can you use > > > > input_api.PresubmitLocalPath() instead maybe? > > > > __file__ doesn't exist when eval()'d > > input_api.PresubmitLocalPath() returns the location of the current PRESUBMIT.py > file, not the location of js_checker.py file, which is what needed here. > > For example when modifying a file inside chrome/browser/resources/settings/, it > would return chrome/browser/resources/ (because that's where the PRESUBMIT.py > file resides). > > Is js_checker.py ever invoked with eval()? I have been using 'git cl presubmit' > to test my changes locally and that line seems to always work as expected. > probably not. if it works for now with `git cl presubmit` it's probably fine
still lgtm, just a nit https://codereview.chromium.org/2872703003/diff/200001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/200001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:79: _HERE_PATH = os_path.dirname(os_path.realpath(__file__)) why do you need to call realpath?
https://codereview.chromium.org/2872703003/diff/200001/tools/web_dev_style/js... File tools/web_dev_style/js_checker.py (right): https://codereview.chromium.org/2872703003/diff/200001/tools/web_dev_style/js... tools/web_dev_style/js_checker.py:79: _HERE_PATH = os_path.dirname(os_path.realpath(__file__)) On 2017/05/25 at 01:01:20, Dan Beam wrote: > why do you need to call realpath? I might not need to, not sure if calling realpath() has any benefit. A quick search (both within Chromium and outside) shows that this is pretty common, For example see https://cs.chromium.org/search/?q=%22os.path.dirname(os.path.realpath(__file_... The equivalent without realpath is also common, https://cs.chromium.org/search/?q=%22os.path.dirname(__file__)%22&type=cs
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dglazkov@chromium.org Link to the patchset: https://codereview.chromium.org/2872703003/#ps200001 (title: "Address comments.")
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": 200001, "attempt_start_ts": 1495733019722580,
"parent_rev": "5475de908ae79acf3a4a09ed5b5e90580d927807", "commit_rev":
"ff82d51d2b29b40486761fb60a2521af5f73e3cb"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Hook up ESLint to web_dev_style PRESUBMIT. Using a trivial .eslintrc.js config for now, just so that the checks can be enabled. More lint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Hook up ESLint to web_dev_style PRESUBMIT. Using a trivial .eslintrc.js config for now, just so that the checks can be enabled. More lint rules will be added in follow up CLs. BUG=720034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2872703003 Cr-Commit-Position: refs/heads/master@{#474720} Committed: https://chromium.googlesource.com/chromium/src/+/ff82d51d2b29b40486761fb60a25... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/ff82d51d2b29b40486761fb60a25... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
