See this discussion: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/uzsXUt9N_KY The first goal of this bug is to introduce .eslintrc config files where needed in order to allow people to run eslint on the devtools code (either from the command line or integrated in their editor) and verify their changes adhere to the agreed style-guide. The second goal is to write documentation about this on the devtools wiki: what's eslint, where to get it, how to install/run, etc... My plan so far is to add: - one config file in browser/devtools, - one config file in toolkit/devtools, - one shared config file for tests (with globals like is, ok, test, ...) + one per test folder that extend from it and add their own specific globals (basically what's in head.js in each folder).

Please see this document for the discussion about rules configuration: https://etherpad.mozilla.org/eslint-devtools

This adds a global .eslintrc file for all source files in /browser/devtools. This file defines a set of global variables that are commonly used in /browser/devtools code. The files that import modules with Cu.import will need to define their own globals as a /* globals ... */ comment. This file alos defines the configuration for all the rules we want to use. This also a set of .eslintrc files, one per test directory in /browser/ devtools. Each of these files extend from one of 2 parent config files: .eslintrc.xpcshell or .eslintrc.mochitest. The parent config define the set of globals these types of tests have access to (test runner functions, assertion functions, etc.). On top of this, each .eslintrc file inside of test folders define their own list of globals for things that are typically in head.js files. Additionally, all head.js files disable the no-unused-vars rule because all they really do is define helper functions without ever using them. Finally, this patch contains a few fixes in various /browser/devtools files to adhere to the rules better.

Comment on attachment 8609334 [details] [diff] [review] bug1166189-eslint.diff Review of attachment 8609334 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/animationinspector/test/.eslintrc @@ +2,5 @@ > + // Extend from the shared list of defined globals for mochitests. > + "extends": "../../.eslintrc.mochitests", > + "globals": { > + // And add specific globals for this test directory. > + "addTab": true, I could see this easily falling out of sync since there's no build step to catch a failure. So remembering to add/remove definitions here would be tough. Should be worried about undefined vars in the test directories at all? Seems likely such a thing would end up causing a test failure anyway. Alternatively, is there a way to point eslint to a file and say "globals defined in this file are OK"? So each test directory could just point to it's own head.js, shared-head.js, and any other helper files.

(In reply to Brian Grinstead [:bgrins] from comment #3) > Comment on attachment 8609334 [details] [diff] [review] > bug1166189-eslint.diff > > Review of attachment 8609334 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/animationinspector/test/.eslintrc > @@ +2,5 @@ > > + // Extend from the shared list of defined globals for mochitests. > > + "extends": "../../.eslintrc.mochitests", > > + "globals": { > > + // And add specific globals for this test directory. > > + "addTab": true, > > I could see this easily falling out of sync since there's no build step to > catch a failure. So remembering to add/remove definitions here would be > tough. Should be worried about undefined vars in the test directories at > all? Seems likely such a thing would end up causing a test failure anyway. > Alternatively, is there a way to point eslint to a file and say "globals > defined in this file are OK"? So each test directory could just point to > it's own head.js, shared-head.js, and any other helper files. You're very right, it occurred to me while I was creating these globals that this was wasted time since it would be too hard to maintain, but then I thought that perhaps if all test folders share a common head.js it would be ok. Anyway, you're probably right, we should just disable whatever rule checks for undefined usages in test folders (no we can't make eslint load a list of globals from a head.js file, this could be a plugin though, not that simple to write, but feasible).

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #4) > (In reply to Brian Grinstead [:bgrins] from comment #3) > > Comment on attachment 8609334 [details] [diff] [review] > > bug1166189-eslint.diff > > > > Review of attachment 8609334 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/devtools/animationinspector/test/.eslintrc > > @@ +2,5 @@ > > > + // Extend from the shared list of defined globals for mochitests. > > > + "extends": "../../.eslintrc.mochitests", > > > + "globals": { > > > + // And add specific globals for this test directory. > > > + "addTab": true, > > > > I could see this easily falling out of sync since there's no build step to > > catch a failure. So remembering to add/remove definitions here would be > > tough. Should be worried about undefined vars in the test directories at > > all? Seems likely such a thing would end up causing a test failure anyway. > > Alternatively, is there a way to point eslint to a file and say "globals > > defined in this file are OK"? So each test directory could just point to > > it's own head.js, shared-head.js, and any other helper files. > You're very right, it occurred to me while I was creating these globals that > this was wasted time since it would be too hard to maintain, but then I > thought that perhaps if all test folders share a common head.js it would be > ok. > Anyway, you're probably right, we should just disable whatever rule checks > for undefined usages in test folders (no we can't make eslint load a list of > globals from a head.js file, this could be a plugin though, not that simple > to write, but feasible). Yeah, I'd say as far as 'bang for the buck' we should remove the test-folder-specific globals for now. As we come up with a better way to manage that (shared head file and/or a plugin to return globals from the head files and/or automated lint failures on push), we can enable that functionality.

Also, I have written a rule for including Cu.imported variables such as Services, XPCOMUtils, LayoutHelpers etc. This means that Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm"); will define LayoutHelpers in scope. It assumes that the imported object's name matches the filename (as per the style guide): ``` // components-imports var escope = require("/usr/local/lib/node_modules/eslint/node_modules/escope"); module.exports = function(context) { return { ExpressionStatement: function(node) { var source = context.getSource(node); var matches = source.match(/^(?:Cu|Components\.utils)\.import\(".*\/(\w+)\.\w+"\);?$/); if (matches) { var name = matches[1]; var scope = context.getScope(); var variables = scope.variables; var variable = new escope.Variable(name, scope); variable.eslintExplicitGlobal = false; variable.writeable = true; variables.push(variable); } } }; }; ```

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6) > Also, I have written a rule for including Cu.imported variables such as > Services, XPCOMUtils, LayoutHelpers etc. > > This means that > Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm"); will define > LayoutHelpers in scope. > > It assumes that the imported object's name matches the filename (as per the > style guide): > > ``` > // components-imports > var escope = > require("/usr/local/lib/node_modules/eslint/node_modules/escope"); > > module.exports = function(context) { > return { > ExpressionStatement: function(node) { > var source = context.getSource(node); > var matches = > source.match(/^(?:Cu|Components\.utils)\.import\(".*\/(\w+)\.\w+"\);?$/); > > if (matches) { > var name = matches[1]; > var scope = context.getScope(); > var variables = scope.variables; > var variable = new escope.Variable(name, scope); > > variable.eslintExplicitGlobal = false; > variable.writeable = true; > > variables.push(variable); > } > } > }; > }; > ``` Thanks Mike, that's awesome. Could you file a follow-up bug for this please?

v2 - A lot more rules defined, and I removed all globals from tests .eslintrc configs, to avoid having to maintain them. Instead, the .eslintrc.mochitest and .eslintrc.xpcshell now disable the rules that check for undefined var usage.

v3 - Now has proper .eslintignore files and code+test in /toolkit/devtools is also taken into account.

From my point of view v3 is reviewable. I can see the following tasks left to be done: - finalize the list of rules and their associated levels here https://etherpad.mozilla.org/eslint-devtools , but we can definitely start landing .eslintrc files throughout the devtools codebase without this being done, - come up with a config for chrome tests (.html files that contain js), but this doesn't necessarily have to be done from the start, - implement a few custom rules (to auto-register globals imported with Cu.import & friends, to enforce spaces in {} but no spaces in [], to check the presence of the license header, to auto-register globals define with Object.defineProperty(this, "aKeyName"), ...), but that should be done in follow-up bugs, - write some documentation on the wiki about how to run this in an IDE or from the command line, - have someone test these configs on another OS, with another IDE. Tom: do you think you could try and run eslint locally on your system, either from the command line or integrated with emacs (which is what you're using, right?), or both, and let me know how that turns out?

I have logged two bugs with ESLint rules in progress: - Bug 1168338 - DevTools ESLint should include globals from Cu.import when linting o Works fine but we would ideally follow the path and check export names. - Bug 1168340 - DevTools ESLint should include globals from head.js when inside devtools tests o Still WIP and depends if some functionality is present in ESLint to extract the globals (I don't see why not). If the second works we may be able to go into required files to get their exports.

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #10) > Tom: do you think you could try and run eslint locally on your system, > either from the command line or integrated with emacs (which is what you're > using, right?), or both, and let me know how that turns out? I tried it both ways; from the command line using the eslint-all-off config from another bug, and from Emacs using the config from this bug. It seems to work fine. For Emacs I ended up mostly following the directions here: http://www.emacswiki.org/emacs/Flycheck#toc2

Comment on attachment 8610138 [details] [diff] [review] bug1166189-eslint v3.diff Review of attachment 8610138 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much. ::: browser/devtools/.eslintrc @@ +22,5 @@ > + }, > + "rules": { > + // These are the rules that have been configured so far to match the > + // devtools coding style. > + // See https://etherpad.mozilla.org/eslint-devtools for rationale behind I think this file should be the sole source of truth, and so I would drop the link to the etherpad. Future changes will be discussed in bugzilla anyway, meaning that, most likely, the etherpad content will rot. I think the comments in this file are already pretty good, so I'd be fine with just dropping the link; but if there are comments worth preserving they could be copied in. That said I don't feel particularly strongly about it. @@ +402,5 @@ > + "operator-assignment": 0, > + // enforce operators to be placed before or after line breaks > + "operator-linebreak": 0, > + // Ensure that the results of typeof are compared against a valid string > + "valid-typeof": 0, On the etherpad the only suggestion for valid-typeof is 2, not 0. Maybe this is one of the edits that happened after you wrote v3. I think it's probably best to land what you have and then someone can do another pass through the etherpad. ::: browser/devtools/.eslintrc.xpcshell @@ +7,5 @@ > + // function do not produce errors. > + "no-unused-vars": [2, {"vars": "local"}], > + // Allow using undefined variables so that tests can refer to functions > + // and variables defined in head.js files, without having to maintain a > + // list of globals in each .eslintrc file. How about a mention of bug 1168340 here? (and in the similar comment in .eslintrc.mochitests)