As part of rolling out ESLint across the tree, we should enable it for dom/cache/test/xpcshell/ There's background on our eslint setups here: https://developer.mozilla.org/docs/ESLint Here's some approximate steps: - In .eslintignore, remove the `dom/cache/test/xpcshell/**` line. - Run eslint: ./mach eslint --fix dom/cache/test/xpcshell/ This should fix most/all of the issues. - Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok. - Create a commit of the work so far. - For the remaining issues, you'll need to fix them by hand. I think most of these will be mozilla/use-services, there are hints on how to fix those here: https://firefox-source-docs.mozilla.org/tools/lint/linters/eslint-plugin-mozilla.html#use-services (you can just run `./mach eslint dom/cache/test/xpcshell/` to check you've fixed them). - Create a second commit of the manual changes and push it to mozreview: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.

FWIW, I've found the eslint rules in other parts of the tree unhelpful and unnecessarily difficult. I'm not sure I really want these turned on for dom/cache.

Tell me how to disable poor eslint rules like this: https://mozilla.logbot.info/devtools/20171026#c13751010 I don't think those rules are helpful at all and just create useless busy work for people. I wasted like 30 minutes to an hour when I ran into in devtools just trying to work around it. I do not want anything like that in dom/cache.

In that case, you've hit a rule that is currently specific to devtools - they are currently a bit stricter than the rest of the mozilla-central. Personally, I'm not sure if we'll ever get a line length limit as there always seems too many cases where it can break. The rules that we currently have for most of mozilla-central are at: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/configs (mainly recommended.js, the test ones are generally extra global information specific to those test areas). There's various ways of disabling rules for a section or line: /* eslint-disable max-len */ <long line> /* eslint-enable max-len */ // eslint-disable-next-line max-len <long line> <long line> // eslint-disable-line max-len More info on https://eslint.org/docs/user-guide/configuring We can also enable/disable per directory if we really need to (though I tend to try and avoid those as I want to get the config as consistent as possible across the tree). The rules we have are a mixture of error detection and stylistic. I could see in future that we could move to formatting via prettier or something (similar to what is currently being moved to with clang-format) for stylistic issues, but that's still future at the moment. If you have other concerns, I'm happy to discuss more.

Personally I do try to wrap lines already. The one I found a bit ridiculous was enforcing the indent on the nested function when I tried to manually wrap the line.

(In reply to Ben Kelly [:bkelly] from comment #4) > Personally I do try to wrap lines already. The one I found a bit ridiculous > was enforcing the indent on the nested function when I tried to manually > wrap the line. Understood, we don't currently have a indent rule that's enforced for the whole tree. If/when we do get near, I suspect we'll have some sort of announcement/discussion about it as it'll affect a lot of code.

Why is this switching all single quotes to double quotes? Is this really a code safety issue? This smells more like busywork to me. And as the main person maintaining dom/cache tests, I prefer single quotes in js.

(In reply to Ben Kelly [:bkelly] from comment #7) > Why is this switching all single quotes to double quotes? Is this really a > code safety issue? This smells more like busywork to me. > > And as the main person maintaining dom/cache tests, I prefer single quotes > in js. This is the general standard we've been using across m-c, and most of the code has it, it was one of the early rules that got adopted for various reasons. This is a more stylistic rule though. I guess we could whitelist / change the rule for just this directory, in some ways I'd like to avoid specific whitelists/differences but I'd rather have ESLint enabled for the other rules. We may want to make things more consistent once we head to consistent formatting across the tree using prettier, but lets deal with that when we come to it. What would you like to do?

Comment on attachment 8938650 [details] Bug 1423841 Enable ESLint for dom/cache/test/xpcshell/ https://reviewboard.mozilla.org/r/209252/#review215112 Thank you Nicola. As per the discussion on the bug, lets wait for Ben to confirm what he wants for the quotes before updating this patch. In the meantime I've added a couple of comments down below. ::: dom/cache/test/xpcshell/head.js:28 (Diff revision 1) > > // Extract a zip file into the profile > function create_test_profile(zipFileName) { > do_get_profile(); > > - var directoryService = Cc['@mozilla.org/file/directory_service;1'] > + Cu.import("resource://gre/modules/Services.jsm"); You only need to import Services.jsm once at the top of the file (after the aliases for Cu are defined) - the form in use here imports into the global scope regardless where it is inserted. ::: dom/cache/test/xpcshell/head.js:29 (Diff revision 1) > // Extract a zip file into the profile > function create_test_profile(zipFileName) { > do_get_profile(); > > - var directoryService = Cc['@mozilla.org/file/directory_service;1'] > - .getService(Ci.nsIProperties); > + Cu.import("resource://gre/modules/Services.jsm"); > + var directoryService = Services.dirsvc; I think we can drop the directoryService shorthand, and just replace the individual instances. ::: dom/cache/test/xpcshell/head.js:81 (Diff revision 1) > zipReader.close(); > } > > -function getCacheDir() > -{ > - let dirService = Cc["@mozilla.org/file/directory_service;1"] > +function getCacheDir() { > + Cu.import("resource://gre/modules/Services.jsm"); > + let dirService = Services.dirsvc; Please drop the dirService shorthand as well. ::: dom/cache/test/xpcshell/make_profile.js:90 (Diff revision 1) > function resetQuotaManager() { > return new Promise(function(resolve) { > - var qm = Cc['@mozilla.org/dom/quota/manager;1'] > + var qm = Cc["@mozilla.org/dom/quota/manager;1"] > .getService(Ci.nsIQuotaManager); > > - var prefService = Cc['@mozilla.org/preferences-service;1'] > + var prefService = Services.prefs; Please drop the prefService intermidate variable.

I'd rather there wasn't a rule for type of quotes at all. Particularly for a tool which doesn't install with ./mach bootstrap and requires extra steps to check locally, I find it quite obnoxious to have patches bounce for checks that aren't actually providing any safety value. It just adds to everyone's busy work for no reason. So if you are asking me if I want this rule on dom/cache, I do not. I don't want eslint at all tbh.

And if you are planning to go further down this road please: 1. Make sure it installs as part of mach bootstrap instead of requiring manual intervention to install required tools. Consider not everyone is on mac or linux or might have different versions of node than you require already installed. 2. Run the lint as part of executing ./mach build or ./mach mochitest so people actually see the failure before landings. 3. Please don't force stylistic rules that don't provide safety value. Checking for semi-colons or braces on single-statement if-statements, sure. Checking for single-quote vs double-quote has no safety value AFAICT.

> Particularly for a tool which doesn't install with ./mach bootstrap and > requires extra steps to check locally, I find it quite obnoxious to have > patches bounce for checks that aren't actually providing any safety value. > It just adds to everyone's busy work for no reason. The value we've found with stylistic issues is that it actually reduces review cycles by avoiding the need for "nits" (when run ahead of landing). It also helps a consistent style around the whole codebase so that developers know what style to write for. Hence its generally been a win for developers & reviewers. I probably should have mentioned that before. > 1. Make sure it installs as part of mach bootstrap instead of requiring > manual intervention to install required tools. Consider not everyone is on > mac or linux or might have different versions of node than you require > already installed. We currently support a wide range of node versions (6.9.1 and later) and MozillaBuild has node installed by default as of a few months ago. We haven't yet got bootstrap support, but that's something we're working on. I agree it isn't ideal, but we're trying to get there as soon as we can. > 2. Run the lint as part of executing ./mach build or ./mach mochitest so > people actually see the failure before landings. We've been writing commit & push hooks that we finally got working reasonably correctly last month, so they aren't yet installed by default, but will be soon. I'm hadn't thought about hooking into build or mochitest, I'll need to think about how that would work (e.g. warn & continue or error). At the moment, I'd prefer to get bootstrap or something initialising editor support (automatic install of plugins or something), which gets the results to the developer even earlier. > So if you are asking me if I want this rule on dom/cache, I do not. I don't > want eslint at all tbh. Since this is test-only code, I'm ok to wontfix this for dom/cache for now at least. Maybe we can review it in 2018 once we've got better ESLint setup & support and it deployed to more places? (and possibly a clearer picture for style rules / the use of prettier).

Editor support is much less useful IMO because of the variety in editors in use. Build and mpxhotest targets are used by everyone. (Personally I use vim without any plugins, so it will not help me.)

Closing as wontfix for now, and we can review later in the year.