Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834433002:
MD Settings: ESLint --fix output, WIP. (Closed)
|
Created:
3 years, 8 months ago by dpapad Modified:
3 years, 7 months ago Reviewers:
CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, dschuyler, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: ESLint --fix output, WIP.
gulp lint --filter=settings --fix
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 27
Patch Set 2 : Relax #
Depends on Patchset: Messages
Total messages: 13 (7 generated)
Description was changed from ========== MD Settings: ESLint --fix output, WIP. BUG= ========== to ========== MD Settings: ESLint --fix output, WIP. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: ESLint --fix output, WIP. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: ESLint --fix output, WIP. gulp lint --filter=settings --fix BUG=None ==========
Current ESLint configuration can be seen at https://codereview.chromium.org/2589333004. I had to turn off various checks for now, to make settings/* code return no errors. We can re-enable those checks little by little as we fix the issues. Also note, we can completely eliminate the use of "gulp" as an intermediary. I just used it because I find it convenient. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/update_warning_dialog.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/update_warning_dialog.js:42: this.$$('#update-warning-message').innerHTML = Single quotes auto fix. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:12: */ OuterIIFEBody indentation fix. Unfortunately --fix is currently not re-indenting the comments properly. I filed https://github.com/eslint/eslint/issues/8478, and it seems that this is addressed in the next version (ESLint 4, which is currently in alhpa). https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (left): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:292: return !!matchingKey ? Route[matchingKey] : null; http://eslint.org/docs/rules/no-extra-boolean-cast fix. We can turn that off if not desired.
Just some observations. Trying to help because I'd like to use the auto formatting :) https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:232: /** @type {!chrome.bluetoothPrivate.SetPairingResponseOptions} */ { This looks like it should be a four space indent. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_page.js:283: })) { The body of the |if| now appears to be dangling. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen_constants.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen_constants.js:33: */ Also missed the comment indent (I assume related to the other issues with comment indention). https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/prefs/prefs.js:288: assert(parts.shift() == 'prefs', 'Path doesn\'t begin with \'prefs\''); I find it more readable to use double quotes in these cases. Imo, the main reason to standardize quotes in languages that allow dictionary['key'] style lookups is for searching the code for those values (like 'key' rather than ['"]key['"]). For an actual string that's not part of the code, it's more readable to use different quotes that escape: e.g. "isn't". https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_printers.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_printers.js:64: {'networkType': chrome.networkingPrivate.NetworkType.ALL, This should be a four space indent I believe. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:143: data: cookie.data}); four space indent
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/update_warning_dialog.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/update_warning_dialog.js:42: this.$$('#update-warning-message').innerHTML = On 2017/04/19 17:48:22, dpapad wrote: > Single quotes auto fix. same in clang-format https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:12: */ On 2017/04/19 17:48:22, dpapad wrote: > OuterIIFEBody indentation fix. Unfortunately --fix is currently not re-indenting > the comments properly. I filed https://github.com/eslint/eslint/issues/8478, and > it seems that this is addressed in the next version (ESLint 4, which is > currently in alhpa). we'd definitely need that to use eslint, IMO... https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/prefs/prefs.js:91: } did you do this or did eslint --fix do this? https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:128: }.bind(this)); i don't necessarily think this indent is better but am OK with it if enforced consistently https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (left): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:292: return !!matchingKey ? Route[matchingKey] : null; On 2017/04/19 17:48:22, dpapad wrote: > http://eslint.org/docs/rules/no-extra-boolean-cast fix. We can turn that off if > not desired. compiler will whine in some cases https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:143: data: cookie.data}); On 2017/04/19 18:58:41, dschuyler wrote: > four space indent well, actually, array and object literals that are multi-line are supposed to have 2\s indent in JS (which differs from C++, for example). this is a continuation, so it should either be "box indented" or 4\s (as Dave said), but I think it'd be more common to see: list.push({ title: ... id: ... data: ... }); or the box-indented version. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (left): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:118: return { websiteUsagePolymerInstance: websiteUsagePolymerInstance, i believe clang-format will remove spaces between Object literals and keys, as in: { blah: blee } becomes {blah: blee} which is actually what we want (it's in either the JS style guide or the chrome-specific one or both, IIRC). does eslint do that?
https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_page.js:283: })) { On 2017/04/19 18:58:41, dschuyler wrote: > The body of the |if| now appears to be dangling. yeah, i think the previous one was better. i think this if () is weird to begin with, though https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/prefs/prefs.js:288: assert(parts.shift() == 'prefs', 'Path doesn\'t begin with \'prefs\''); On 2017/04/19 18:58:41, dschuyler wrote: > I find it more readable to use double quotes in these cases. > Imo, the main reason to standardize quotes in languages that allow > dictionary['key'] style lookups is for searching the code for those values (like > 'key' rather than ['"]key['"]). For an actual string that's not part of the > code, it's more readable to use different quotes that escape: e.g. "isn't". +1, not sure what clang-format does here not the end of the world https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_printers.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_printers.js:64: {'networkType': chrome.networkingPrivate.NetworkType.ALL, On 2017/04/19 18:58:41, dschuyler wrote: > This should be a four space indent I believe. yeah, i think this goes against our style guide
Thanks for the initial comments. Posted some responses. I think most of the comments are addressable by tweaking the configuration. Will play with it a bit more. I believe that we can reach the desired output with the right configuration. After that, we can start tightening down our config with extra checks as we see fit (if we agree that we want to use ESLint ofcourse). https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_page.js:283: })) { On 2017/04/19 at 19:07:03, Dan Beam wrote: > On 2017/04/19 18:58:41, dschuyler wrote: > > The body of the |if| now appears to be dangling. > > yeah, i think the previous one was better. i think this if () is weird to begin with, though This is also configurable. Specifically see http://eslint.org/docs/rules/indent, FunctionDeclaration and FunctionExpression subpotions. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen_constants.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen_constants.js:33: */ On 2017/04/19 at 18:58:41, dschuyler wrote: > Also missed the comment indent (I assume related to the other issues with comment indention). Yes, that is the same issue mentioned before. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/prefs/prefs.js:91: } On 2017/04/19 at 19:04:40, Dan Beam wrote: > did you do this or did eslint --fix do this? I didn't do any manual text editing in this CL. This is the output of --fix. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_printers.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_printers.js:64: {'networkType': chrome.networkingPrivate.NetworkType.ALL, On 2017/04/19 at 19:07:03, Dan Beam wrote: > On 2017/04/19 18:58:41, dschuyler wrote: > > This should be a four space indent I believe. > > yeah, i think this goes against our style guide This is configurable, see http://eslint.org/docs/rules/object-curly-newline. I'll play with the options and retry --fix. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:143: data: cookie.data}); On 2017/04/19 at 19:04:40, Dan Beam wrote: > On 2017/04/19 18:58:41, dschuyler wrote: > > four space indent > > well, actually, array and object literals that are multi-line are supposed to have 2\s indent in JS (which differs from C++, for example). > > this is a continuation, so it should either be "box indented" or 4\s (as Dave said), but I think it'd be more common to see: > > list.push({ > title: ... > id: ... > data: ... > }); > > or the box-indented version. Will tweak config again. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/website_usage_private_api.js (left): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/website_usage_private_api.js:118: return { websiteUsagePolymerInstance: websiteUsagePolymerInstance, On 2017/04/19 at 19:04:40, Dan Beam wrote: > i believe clang-format will remove spaces between Object literals and keys, as in: > > { blah: blee } > > becomes > > {blah: blee} > > which is actually what we want (it's in either the JS style guide or the chrome-specific one or both, IIRC). > > does eslint do that? Yes, see http://eslint.org/docs/rules/object-curly-newline and object-curly-spacing rules.
Description was changed from ========== MD Settings: ESLint --fix output, WIP. gulp lint --filter=settings --fix BUG=None ========== to ========== MD Settings: ESLint --fix output, WIP. gulp lint --filter=settings --fix BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== MD Settings: ESLint --fix output, WIP. gulp lint --filter=settings --fix BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: ESLint --fix output, WIP. gulp lint --filter=settings --fix BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
I have relaxed the checks quite a bit for now (see config at https://codereview.chromium.org/2589333004/diff/140001/chrome/browser/resourc...). Turning off 'indent' checks basically reduced the amount of errors dramatically. As discussed previously, I'll let clang-format handle indenting for now, and let ESLint check other stuff (stuff that is not checked by clang-format). If this output looks reasonable, I'll look onto 1) Add ESLint to our NPM deps. 2) Add PRESUBMIT magic so that it only runs on affected files. Slowly roll it out from MD Settings, to other UI pages. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/prefs/prefs.js:91: } On 2017/04/19 at 19:19:55, dpapad wrote: > On 2017/04/19 at 19:04:40, Dan Beam wrote: > > did you do this or did eslint --fix do this? > > I didn't do any manual text editing in this CL. This is the output of --fix. So this was done because of http://eslint.org/docs/rules/no-extra-semi, which is included in the default "eslint:recommended" configuration. https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (left): https://codereview.chromium.org/2834433002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:292: return !!matchingKey ? Route[matchingKey] : null; On 2017/04/19 at 19:04:40, Dan Beam wrote: > On 2017/04/19 17:48:22, dpapad wrote: > > http://eslint.org/docs/rules/no-extra-boolean-cast fix. We can turn that off if > > not desired. > > compiler will whine in some cases Done, turned this off for now. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
