As we get automation going it will be important to have the tree pass mach eslint. At the moment devtools fails a lot so we either need to reduce their ruleset or add more ignores.

The general consensus in the team was (I think) to disable rules to get eslint passing, and then re-enable them one by one. I see that this bug blocks the meta bug 1229856. Shouldn't it block a more specific automation bug instead? Maybe bug 1229588? I guess we need to do this only once we have a concrete plan to automate eslint. Once we do do it, we'll need a tracking bug to re-enable the rules: bug 1230070.

I was running eslint on the whole devtools directory and realized that one of our custom rules was broken: import-headjs-globals. This rule isn't really a rule as it doesn't report any errors, instead if looks at the head.js file in the same dir and import globals from there in scope. So, with bug 1230093 and bug 1230095 fixed, we should have far less eslint errors.

The other quick thing we can fix before disabling rules is that devtools/client/aboutdebugging/test doesn't have a proper .eslintrc file (which should inherit from devtools/.eslintrc.mochitests in order to have the proper test globals defined).

Ah, in fact there are other missing test .eslintrc files: - devtools/shared/worker/tests/browser/ - devtools/shared/tests/browser/ and in other places for xpcshell tests. I'll submit a new patch to add those. I think we have about 32000 errors in the devtools code with those fixed. Among these errors, I believe some could just be turned to warning instead of disabled entirely: - indent: we have about 6000 of them, this makes no sense to me as an error - comma-spacing: this is also a formatting-only issue, and we have 2000 of them - brace-style: we have 2700 of them, this might also be a good one to change to a warning instead, at least for now - camelcase: same, about 2700 of them - ... With this fixed locally, I was able to go down to about 18000 errors. Another candidate for a quick fix is updating the .eslintignore which isn't correct today. We're not ignoring tern from the right folder.

Comment on attachment 8695220 [details] [diff] [review] Bug_1229859_-_Add_the_expected_mochitest_.eslintrc.diff Going to provide a bigger patch that includes this one.

So I'm down to ~9000 errors now which is still a lot, but considering I had 32000 1 hour ago, it's not so bad. These were only low-hanging fruits however. The next ones are going to be harder. So, as said previously, we might decide to disable the remaining failing rules for the sake of passing when we have eslint automated, and then re-enabling them one by one in bug 1230070. I went quickly over the remaining failures are the large majority is from the no-undef rule. Here's what's happening: We have many files that just assume global variables to be available. Let's take the xample of devtools/client/canvasdebugger/callslist.js It uses, among other things, the following global vars: Heritage, WidgetMethods, SideMenuWidget, $, document, etc. These things are deinfed in devtools/cient/canvasdebugger.js, and both these files are loaded via <script> tags in devtools/client/canvasdebugger/canvasdebugger.xul So when the tool loads (in an iframe), the globals defined by canvasdebugger.js get added to the window and any other script loaded in the window after this can safely use them. Only, ESLint doesn't know about this and therefore complains about the undefined variables. The hard thing here is that *not* all our files are loaded this way, many are loaded via a module loader and so they specifically define the imports they need, no problem there. But for many of our tools, we at least have a couple of files that don't do this. One solution is to manually maintain the list of globals in a comment inside the file: /* globals: Heritage, WidgetMethods, SideMenuWidget, $, document */ But that's manual hence hard to maintain. Maybe we could write a custom eslint rule just for these files. Maybe this rule could use a comment like: /* use-globals-from: canvasdebugger.js */ if it sees this comment, then it loads that file, and adds all globals from there to the current scope.

Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8) > Created attachment 8695244 [details] > MozReview Request: Bug 1229859 - Massively reduce the number of eslint > errors in devtools by ignoring lib files, adding missing .eslintrc files and > making some rules be warnings; r?Mossop > > Bug 1229859 - Massively reduce the number of eslint errors in devtools by > ignoring lib files, adding missing .eslintrc files and making some rules be > warnings; r?Mossop Damn, I just realized I have committed an unrelated change to import-headjs-globals.js (I had done that change locally just so the rule would work correctly when ran from mach (see bug 1230095)). Please ignore that change.

https://reviewboard.mozilla.org/r/27031/#review24425 ::: testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js:94 (Diff revision 1) > - var testPath = this.getFilename(); > + var fullTestPath = this.getFilename(); Please ignore this change. I will get it removed. This is unrelated.

Comment on attachment 8695244 [details] MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27031/diff/1-2/

Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop

Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r?Mossop

The last 2 commits remove another 500 to 1000 errors thanks to the new rule I described in comment 7. The goal was not to enable this rule *everywhere* yet but to fix the most common offenders. There are some more tricky cases in devtools/client/debugger to deal with, and also, the rule should be made better incrementally (like add global variables from Object.define, Cu.import and lazyLoaders, ...)

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #5) > Ah, in fact there are other missing test .eslintrc files: > - devtools/shared/worker/tests/browser/ > - devtools/shared/tests/browser/ > and in other places for xpcshell tests. I ran across this today and made a list. See bug 1230195.

Comment on attachment 8695244 [details] MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop https://reviewboard.mozilla.org/r/27031/#review24473 ::: devtools/.eslintrc:39 (Diff revision 2) > - "brace-style": [2, "1tbs", {"allowSingleLine": false}], > + "brace-style": [1, "1tbs", {"allowSingleLine": false}], You are of course welcome to do whatever you like here however I'll note that in browser and toolkit we're probably not going to do anything as warnings. If you don't care about the rule being followed then there isn't much point spamming developers with it.

Comment on attachment 8695291 [details] MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r?Mossop https://reviewboard.mozilla.org/r/27053/#review24475

Comment on attachment 8695290 [details] MozReview Request: Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop https://reviewboard.mozilla.org/r/27051/#review24477 ::: testing/eslint-plugin-mozilla/docs/import-globals-from.rst:10 (Diff revision 1) > +When the "import-globals-from: <path>" comment is found in a file, then all This comment suggests you need a : for this to work ::: testing/eslint-plugin-mozilla/docs/index.rst:13 (Diff revision 1) > +``import-globals-from`` When the "import-globals-from: <path>" comment is found This comment suggests you need a : for it to work ::: testing/eslint-plugin-mozilla/lib/rules/import-globals-from.js:43 (Diff revision 1) > + } This should be removed ::: testing/eslint-plugin-mozilla/lib/rules/import-globals-from.js:55 (Diff revision 1) > + var match = /^import-globals-from[\s:]*(.*)$/.exec(value); This rule matches "import-globals-fromfoobar" which it probably shouldn't. I'd just do /^import-globals-from\s+(.*)$/

(In reply to Dave Townsend [:mossop] from comment #17) > Comment on attachment 8695244 [details] > MozReview Request: Bug 1229859 - Massively reduce the number of eslint > errors in devtools by ignoring lib files, adding missing .eslintrc files and > making some rules be warnings; r?Mossop > > https://reviewboard.mozilla.org/r/27031/#review24473 > > ::: devtools/.eslintrc:39 > (Diff revision 2) > > - "brace-style": [2, "1tbs", {"allowSingleLine": false}], > > + "brace-style": [1, "1tbs", {"allowSingleLine": false}], > > You are of course welcome to do whatever you like here however I'll note > that in browser and toolkit we're probably not going to do anything as > warnings. If you don't care about the rule being followed then there isn't > much point spamming developers with it. Yup, I agree. I'd say let's keep 1 for rules we want to keep as errors in the future but are temporarily disabling for the sake of automating.

Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r=Mossop

Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop

Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop

Comment on attachment 8695811 [details] MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r=Mossop https://reviewboard.mozilla.org/r/27195/#review24587 This was already R+

Comment on attachment 8695813 [details] MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop https://reviewboard.mozilla.org/r/27199/#review24589 This was already R+

Comment on attachment 8695813 [details] MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop Really sorry about the noise on this bug. I pushed to mozreview from a different computer, and it created a new review entirely, didn't update the commits in the same review. Also, even if setting "r=Mossop" in the message, it didn't mark those 2 commits as R+. I guess I should have pushed only the one changeset that needed re-review only.

Comment on attachment 8695812 [details] MozReview Request: Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop https://reviewboard.mozilla.org/r/27197/#review24805

The next step for this bug would be to add devtools files to .eslintignore so that `./mach eslint` at root level passes. Then we can re-enable eslint file by file, which is an easier unit to deal with than rule by rule on the whole devtools code base. Also re-enabling eslint for a single file is really nice good-first-bug.

https://hg.mozilla.org/mozilla-central/rev/c857a4608700 https://hg.mozilla.org/mozilla-central/rev/760f2be9b549 https://hg.mozilla.org/mozilla-central/rev/66b69aaa8cc3

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #31) > The next step for this bug would be to add devtools files to .eslintignore > so that `./mach eslint` at root level passes. > Then we can re-enable eslint file by file, which is an easier unit to deal > with than rule by rule on the whole devtools code base. > Also re-enabling eslint for a single file is really nice good-first-bug. Bug 1231956 and bug 1231957 logged.

We discussed this on irc and agreed that this bug is fixed, and that bug 1231957 is the new meta-bug for fixing eslint warnings in devtools.

Actually close it.