ESlint 2 made a number of changes that break our plugin and configs. Many other projects are switching and I'm hitting issues with having the wrong version installed

Since we're already relying on internals it makes more sense to just use escope's function for declaring variables. We then have to remove references from the through array and update them to point to their variables, this was cribbed from https://github.com/eslint/eslint/blob/master/lib/eslint.js#L180 Review commit: https://reviewboard.mozilla.org/r/40533/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40533/

For some reason the eslint job isn't picking these up... Review commit: https://reviewboard.mozilla.org/r/40535/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40535/

We still fail in eslint 2, some rules have gotten better at spotting problems and some rules have been replaced. The latter is hard to fix without either removing the rules or dropping them to warnings which is basically the same thing or dropping support for eslint 1.x. Any thoughts?

(In reply to Dave Townsend [:mossop] from comment #2) > Created attachment 8731389 [details] > MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs > > For some reason the eslint job isn't picking these up... > > Review commit: https://reviewboard.mozilla.org/r/40535/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/40535/ Turns out it isn't picking them up because the html plugin we use in automation is older and doesn't detect these issues apparently.

Comment on attachment 8731389 [details] MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs https://reviewboard.mozilla.org/r/40535/#review37235 ::: toolkit/content/plugins.html:144 (Diff revision 1) > var stateDd = document.createElement("dd"); > var state = document.createElement("span"); > state.setAttribute("label", "state"); > state.appendChild(document.createTextNode(pluginsbundle.GetStringFromName("state_label") + " ")); > stateDd.appendChild(state); > - status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled"); > + var status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled"); This is a bug in eslint 2, which we should probably file: https://developer.mozilla.org/en-US/docs/Web/API/Window/status as with any property of window, you can access it from anywhere running with that window as the global object without prefixing it with "window". For more such confusing fun, see e.g. https://github.com/mozilla/readability/issues/272 . rs=me to fix it here anyway, because obviously we don't actually *intend* to assign to window.status... Maybe eslint has a separate warning setting for this? That would be useful, actually...

(In reply to Dave Townsend [:mossop] from comment #3) > We still fail in eslint 2, some rules have gotten better at spotting > problems and some rules have been replaced. The latter is hard to fix > without either removing the rules or dropping them to warnings which is > basically the same thing or dropping support for eslint 1.x. Any thoughts? I was under the impression that the rules that have been replaced in 2.x have equivalent rules that more or less map one to one with the old rules. I'm not a fan of changing the levels to warnings because then no one really cares anymore as the builds don't fail and it's just noise in the logs (like, when you do have, say, 1 eslint error on try, then it's lost in hundreds of warnings). How much work are we talking about for fixing errors related to the replaced rules?

Comment on attachment 8731388 [details] MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x and eslint 2.x. r?pbrosset https://reviewboard.mozilla.org/r/40533/#review37281 Thanks for fiding a way to fix this with eslint 2.x. It's unfortunate that eslint still doesn't provide an officially supported way to register new globals from rules. So we still have to do really hacky things to get it working. Could you please add a comment at the top of the function you changed explaining this? That there's no clean way of doing this, and that this is likely to break in the future if eslint's internals change again.

(In reply to Patrick Brosset [:pbro] from comment #6) > (In reply to Dave Townsend [:mossop] from comment #3) > > We still fail in eslint 2, some rules have gotten better at spotting > > problems and some rules have been replaced. The latter is hard to fix > > without either removing the rules or dropping them to warnings which is > > basically the same thing or dropping support for eslint 1.x. Any thoughts? > I was under the impression that the rules that have been replaced in 2.x > have equivalent rules that more or less map one to one with the old rules. > I'm not a fan of changing the levels to warnings because then no one really > cares anymore as the builds don't fail and it's just noise in the logs > (like, when you do have, say, 1 eslint error on try, then it's lost in > hundreds of warnings). > How much work are we talking about for fixing errors related to the replaced > rules? Switching to the replaced rules is easy, like you say they are pretty much a 1:1 match, but then if you try to use eslint 1.x it throws errors about unknown rules. That's fine but will confuse folks who are still running 1.x. We might want to implement an eslint version check in mach to warn about that. The harder challenge is the updated rules, consistent-return and generator-star-spacing are flagging a lot more errors than previously, the former is now spotting cases where a function returns something in some cases but then runs to completion without returning in others. That's a valid issue and unfortunately requires manual work. I haven't looked into why the other is flagging so much yet but at least that is automatically fixable.

(In reply to Patrick Brosset [:pbro] from comment #7) > Comment on attachment 8731388 [details] > MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x > and eslint 2.x. r?pbrosset > > https://reviewboard.mozilla.org/r/40533/#review37281 > > Thanks for fiding a way to fix this with eslint 2.x. > It's unfortunate that eslint still doesn't provide an officially supported > way to register new globals from rules. So we still have to do really hacky > things to get it working. > Could you please add a comment at the top of the function you changed > explaining this? That there's no clean way of doing this, and that this is > likely to break in the future if eslint's internals change again. One idea that occurred to me is that rather than using a rule for this we could use a custom JS parser based on espree. That would let us change the "Cu.import" AST nodes into "var foo" nodes before eslint even sees them. That would probably give us better insulation from eslint internal changes.

(In reply to Patrick Brosset [:pbro] from comment #7) > Could you please add a comment at the top of the function you changed > explaining this? That there's no clean way of doing this, and that this is > likely to break in the future if eslint's internals change again. That comment already exists from when Mike first wrote the original code.(In reply to :Gijs Kruitbosch from comment #5) > Comment on attachment 8731389 [details] > MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs > > https://reviewboard.mozilla.org/r/40535/#review37235 > > ::: toolkit/content/plugins.html:144 > (Diff revision 1) > > var stateDd = document.createElement("dd"); > > var state = document.createElement("span"); > > state.setAttribute("label", "state"); > > state.appendChild(document.createTextNode(pluginsbundle.GetStringFromName("state_label") + " ")); > > stateDd.appendChild(state); > > - status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled"); > > + var status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled"); > > This is a bug in eslint 2, which we should probably file: > > https://developer.mozilla.org/en-US/docs/Web/API/Window/status https://github.com/sindresorhus/globals/pull/79

https://hg.mozilla.org/integration/fx-team/rev/9a7f99aa5607 https://hg.mozilla.org/integration/fx-team/rev/090b7c54cbb2

(In reply to Dave Townsend [:mossop] from comment #9) > (In reply to Patrick Brosset [:pbro] from comment #7) > > Comment on attachment 8731388 [details] > > MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x > > and eslint 2.x. r?pbrosset > > > > https://reviewboard.mozilla.org/r/40533/#review37281 > > > > Thanks for fiding a way to fix this with eslint 2.x. > > It's unfortunate that eslint still doesn't provide an officially supported > > way to register new globals from rules. So we still have to do really hacky > > things to get it working. > > Could you please add a comment at the top of the function you changed > > explaining this? That there's no clean way of doing this, and that this is > > likely to break in the future if eslint's internals change again. > > One idea that occurred to me is that rather than using a rule for this we > could use a custom JS parser based on espree. That would let us change the > "Cu.import" AST nodes into "var foo" nodes before eslint even sees them. > That would probably give us better insulation from eslint internal changes. That would be pretty cool! We'd have to be careful with line numbers though, but that seems like a small problem. Could eslint pre-processors (I forget what they are called) be used for this?

(In reply to Patrick Brosset [:pbro] from comment #12) > (In reply to Dave Townsend [:mossop] from comment #9) > > (In reply to Patrick Brosset [:pbro] from comment #7) > > > Comment on attachment 8731388 [details] > > > MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x > > > and eslint 2.x. r?pbrosset > > > > > > https://reviewboard.mozilla.org/r/40533/#review37281 > > > > > > Thanks for fiding a way to fix this with eslint 2.x. > > > It's unfortunate that eslint still doesn't provide an officially supported > > > way to register new globals from rules. So we still have to do really hacky > > > things to get it working. > > > Could you please add a comment at the top of the function you changed > > > explaining this? That there's no clean way of doing this, and that this is > > > likely to break in the future if eslint's internals change again. > > > > One idea that occurred to me is that rather than using a rule for this we > > could use a custom JS parser based on espree. That would let us change the > > "Cu.import" AST nodes into "var foo" nodes before eslint even sees them. > > That would probably give us better insulation from eslint internal changes. > That would be pretty cool! We'd have to be careful with line numbers though, > but that seems like a small problem. Could eslint pre-processors (I forget > what they are called) be used for this? No need. You can override the parser in the eslintrc config (many folks use babel-eslint as a parser to support more modern JS than espree does). Each node includes it line number in it I think so we'd just make declaration nodes with the same lines as the Cu.import nodes I guess. A little harder when the Cu.import is somewhere within a function but still declaring globals, same goes for the comment directives we use to add globals so there is a bit of work there to get it right otherwise I might have tried already! Worth a thought though.

Review commit: https://reviewboard.mozilla.org/r/40855/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40855/

This imports semantic_version from https://github.com/rbarrois/python-semanticversion/tree/v2.5.0 so we can verify the version of installed npm modules against a semantic version requirement. Review commit: https://reviewboard.mozilla.org/r/40857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40857/

Review commit: https://reviewboard.mozilla.org/r/40859/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40859/

Review commit: https://reviewboard.mozilla.org/r/40861/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40861/

Review commit: https://reviewboard.mozilla.org/r/40863/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40863/

Apparently with eslint 2 generator-star-spacing started checking anonymous functions as well as regular ones, that is why so many new errors appeared. I put a bunch of updates into mozreview including switching versions in mach eslint (which pre-run version checks!) and the lint-on-checking task and updating a bunch of the fixes. I'm not likely to have much time to do more at the moment though and we can't land this piecemeal. If anyone feels like fixing more of the failures please feel free to take over!

https://hg.mozilla.org/mozilla-central/rev/9a7f99aa5607 https://hg.mozilla.org/mozilla-central/rev/090b7c54cbb2

Review commit: https://reviewboard.mozilla.org/r/44613/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44613/

For now this means turning off keyword-spacing. This replaced the old space-before-keywords and space-after-keywords but for some reason now notices that our spacing after some keywords, particularly catch, switch and while is very inconsistent. Correcting one way or another is time consuming because eslint's auto-fixing can't work on html or xbl files. Review commit: https://reviewboard.mozilla.org/r/44615/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44615/

Most of this is fixing functions that in some cases return a value but then can also run to completion without returning anything. ESLint 2 catches this where previous versions didn't. Unless there was an obvious other choice I just made these functions return undefined at the end which is effectively what already happens. Review commit: https://reviewboard.mozilla.org/r/44617/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44617/

Most of this is fixing functions that in some cases return a value but then can also run to completion without returning anything. ESLint 2 catches this where previous versions didn't. Unless there was an obvious other choice I just made these functions return undefined at the end which is effectively what already happens. Review commit: https://reviewboard.mozilla.org/r/44619/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44619/

ESLint 2 now flags anonymous generator functions that don't match the generator-star-spacing rule so this mostly is fixing that. Review commit: https://reviewboard.mozilla.org/r/44621/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44621/

ESLint 2 now flags anonymous generator functions according to generator-star-spacing. Most of the changes here are correcting that. Review commit: https://reviewboard.mozilla.org/r/44623/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44623/

Comment on attachment 8731821 [details] MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40857/diff/1-2/

Comment on attachment 8731820 [details] MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40855/diff/1-2/

Comment on attachment 8731822 [details] MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40859/diff/1-2/

With the latest set of patches everything passes eslint 2 except for two devtools files where it is flagging an error on functions that are too complex. Could you find someone to fix those Patrick?

Comment on attachment 8731822 [details] MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs https://reviewboard.mozilla.org/r/40859/#review41323 ::: toolkit/components/aboutmemory/content/aboutMemory.js:1171 (Diff revision 2) > case UNITS_COUNT_CUMULATIVE: return formatInt(this._amount); > case UNITS_PERCENTAGE: return formatPercentage(this._amount); > default: > assertInput(false, "bad units in TreeNode.toString"); > } > + return undefined; The default case in the switch statement actually throws indirectly. Can you just do that directly instead to shut this up? throw new Error("bad units in TreeNode.toString"); will also get us a stack here if anyone ever hits this. ::: toolkit/components/asyncshutdown/AsyncShutdown.jsm:224 (Diff revision 2) > > function debug(msg, error=null) { > if (DEBUG_LOG) { > return log(msg, "DEBUG: ", error); > } > + return undefined; log() has no return value. Just stop returning its rv here and below. ::: toolkit/components/ctypes/tests/unit/head.js:77 (Diff revision 2) > bsource = b.toSource(); > finished = true; > } catch (x) { > } > if (finished) { > return do_check_eq(asource, bsource); do_check_eq throws, so you can just replace this with: do_check_eq(asource, bsource); return; which will avoid having to return undefined; ::: toolkit/components/perfmonitoring/PerformanceStats.jsm:653 (Diff revision 2) > let probe = Probes[probeName]; > if (!probe.isEqual(this[probeName], to[probeName])) { > return false; > } > } > + return undefined; Um. Yeah. Maybe return true? It looks like that's what it's supposed to do and it accidentally got removed in https://hg.mozilla.org/mozilla-central/rev/ca96c76db6a2 . ::: toolkit/content/tests/browser/browser_findbar.js:159 (Diff revision 2) > > yield promiseFindFinished("s", false); > ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty"); > > yield BrowserTestUtils.removeTab(tab); > + return undefined; Please change the earlier 'return true' to just 'return' ::: toolkit/content/tests/browser/browser_findbar.js:199 (Diff revision 2) > is(document.activeElement, findBar._findField.inputField, > "findbar is now focused"); > is(findBar._findField.value, "abc", "abc fully entered as find query"); > > yield BrowserTestUtils.removeTab(tab); > + return undefined; Ditto. ::: toolkit/content/tests/browser/head.js:24 (Diff revision 2) > } > findbar.removeEventListener("transitionend", cont); > resolve(); > }); > findbar.close(); > + return undefined; Please brace the first if statement and stick `resolve()` and `return` on separate lines. ::: toolkit/modules/Log.jsm:371 (Diff revision 2) > level = this.level; > } > > params.action = action; > this.log(level, params._message, params); > + return undefined; Stick this.log() on a separate line from the early return.

Comment on attachment 8738620 [details] MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN https://reviewboard.mozilla.org/r/44615/#review41357 I think there's something missing in this patch as all I see changing in .eslintrc is commented code which doesn't match the commit message.

https://reviewboard.mozilla.org/r/44615/#review41357 Oh you're right, I didn't notice that the deprecated rules were already commented out. No wonder I saw so many errors trying to turn on keyword-spacing!. So basically all this patch does is remove the commented out old ESLint 1 rules and replace them with their replacement still commented out. Also turns off linting for microformat tests which are external code.

Comment on attachment 8731820 [details] MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal https://reviewboard.mozilla.org/r/40855/#review41363

Comment on attachment 8738620 [details] MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN https://reviewboard.mozilla.org/r/44615/#review41365

https://reviewboard.mozilla.org/r/44615/#review41367 ::: toolkit/.eslintrc:46 (Diff revision 1) > + // Require spaces before and after keywords > + // "keyword-spacing": 2, > + Please update the commit message to be more accurate now.

Comment on attachment 8738623 [details] MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag https://reviewboard.mozilla.org/r/44621/#review41375 ::: browser/components/extensions/test/browser/file_popup_api_injection_a.html:6 (Diff revision 1) > <!DOCTYPE html> > <html> > <head> > <meta charset="utf-8"> > <script type="application/javascript"> > + /* eslint strict: "off" */ I'd rather we just add "use strict" ::: browser/components/extensions/test/browser/file_popup_api_injection_b.html:6 (Diff revision 1) > <!DOCTYPE html> > <html> > <head> > <meta charset="utf-8"> > <script type="application/javascript"> > + /* eslint strict: "off" */ Same here. ::: browser/components/extensions/test/browser/head.js (Diff revision 1) > if (group.areaType == CustomizableUI.TYPE_TOOLBAR) { > return win.document.getElementById("customizationui-widget-panel"); > } else { > return win.PanelUI.panel; > } > - return null; Let's also get rid of the `else` ::: toolkit/components/extensions/.eslintrc:343 (Diff revision 1) > > // Allow comments inline after code. > "no-inline-comments": 0, > > + // Disallow use of labels for anything other then loops and switches. > + "no-labels": 2, This should have `{"allowLoop": true}`

Comment on attachment 8738622 [details] MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe https://reviewboard.mozilla.org/r/44619/#review41371 ::: browser/base/content/browser-plugins.js (Diff revision 1) > MESSAGES: [ > "PluginContent:ShowClickToPlayNotification", > "PluginContent:RemoveNotification", > "PluginContent:UpdateHiddenPluginUI", > "PluginContent:HideNotificationBar", > - "PluginContent:ShowInstallNotification", is this part of another patch? or are you removing something that was unused to simplify the fix for receiveMessage below? ::: browser/base/content/browser.js:3352 (Diff revision 1) > > case "Link:AddSearch": > this.addSearch(aMsg.target, aMsg.data.engine, aMsg.data.url); > break; > } > + return undefined; why is this one necessary? I don't see anything else returning on this function ::: browser/base/content/sync/setup.js:124 (Diff revision 1) > startNewAccountSetup: function () { > if (!Weave.Utils.ensureMPUnlocked()) > return false; > this._settingUpNew = true; > this.wizard.pageIndex = NEW_ACCOUNT_START_PAGE; > + return undefined; here (and the function below), I believe it's better to just replace the early `return false;` with just `return;`. These two are called from oncommand="foo()" handlers. Do you know if there are differences in behavior from a function returning false and not returning? ::: browser/components/customizableui/CustomizableUI.jsm:2137 (Diff revision 1) > aWindow.gNavToolbox.dispatchEvent(evt); > }, > > dispatchToolboxEvent: function(aEventType, aDetails={}, aWindow=null) { > if (aWindow) { > return this._dispatchToolboxEventToWindow(aEventType, aDetails, aWindow); `this._dispatchToolboxEventToWindow` doesn't return anything, so it's better to just split these line in two to put this early return as a standalone line.

Comment on attachment 8738619 [details] MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka https://reviewboard.mozilla.org/r/44613/#review41441 LGTM. Thanks for doing this!

Comment on attachment 8738621 [details] MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan https://reviewboard.mozilla.org/r/44617/#review41443 ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:668 (Diff revision 1) > }, aSendPerformance, aTimeout)); > > // Always call AddonManager updateAddonRepositoryData after we refill the cache > yield new Promise((resolve, reject) => > AddonManagerPrivate.updateAddonRepositoryData(resolve)); > + return undefined; If I read this right, it looks like the `return this._clearCache();` lines in the body above could be changed to `yield this._clearCache(); return;` and that might be clearer? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3057 (Diff revision 1) > } > } > > yield systemAddonLocation.cleanDirectories(); > } > + return undefined; basically same as above, if we change `return systemAddonLocation.cleanDirectories();` to `yield ...; return;` it seems to be easier to read and understand.

Comment on attachment 8738624 [details] MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro https://reviewboard.mozilla.org/r/44623/#review41497 ::: devtools/client/animationinspector/test/head.js:217 (Diff revision 1) > -var getAnimationPlayerState = Task.async(function*(selector, > +var getAnimationPlayerState = Task.async(function* (selector, > animationIndex = 0) { `animationIndex = 0` on the second line was supposed to align vertically with `selector`. Now that there is an extra space, it doesn't anymore. Can you insert a new space on the second line? ::: devtools/client/animationinspector/test/head.js:268 (Diff revision 1) > - timeline.once("timeline-data-changed", () => hasMoved = true); > + timeline.once("timeline-data-changed", () => { > + hasMoved = true; > + }); Looks like rule no-return-assign in eslint 2 now also checks arrow functions. Honestly, I don't know if this is what we want. Arrow functions are very often used (like here) as a very compact way to define behavior. And having to wrap this on 3 lines is too bad. `() => hasMoved = true` is short and sweet, and fits nicely in a one liner. This is the beauty of arrow functions. So, either we disable this rule for devtools, or we do this one change (I haven't yet reviewed everything, but I doubt that there are many similar fixes in this commit). Ok, disabling the rule has way more impact than keeping these few fixes, so forget about this. We can revisit this rule in devtools later if it frustrate people. ::: devtools/client/inspector/markup/test/browser_markup_keybindings_delete_attributes.js:10 (Diff revision 1) > "use strict"; > > // Tests that attributes can be deleted from the markup-view with the delete key > // when they are focused. > > -const HTML = `<div id="id" class="class" data-id="id"></div>`; > +const HTML = "<div id=\"id\" class=\"class\" data-id=\"id\"></div>"; We have the "avoid-escape" especially for this, so you could simply replace the backticks with single quotes instead: ``` const HTML = '<div id="id" class="class" data-id="id"></div>'; ``` ::: devtools/client/inspector/rules/test/head.js:662 (Diff revision 1) > -var addProperty = Task.async(function*(view, ruleIndex, name, value, > +var addProperty = Task.async(function* (view, ruleIndex, name, value, > commitValueWith = "VK_RETURN", > blurNewProperty = true) { Same comment about vertically aligning these arguments. Please insert a space on the 2 lines that wrap below the function definition. ::: devtools/client/inspector/rules/test/head.js:720 (Diff revision 1) > -var setProperty = Task.async(function*(view, textProp, value, > +var setProperty = Task.async(function* (view, textProp, value, > blurNewProperty = true) { ditto ::: devtools/client/inspector/rules/test/head.js:753 (Diff revision 1) > -var removeProperty = Task.async(function*(view, textProp, > +var removeProperty = Task.async(function* (view, textProp, > blurNewProperty = true) { ditto Now that I'm done reviewing, I counted 6 places where you had to wrap an arrow function on 3 lines because of the no-return-assign rule change. Knowing the amount of code in devtools, this is a small number. So let's keep it as you did.

(In reply to Dave Townsend [:mossop] from comment #30) > With the latest set of patches everything passes eslint 2 except for two > devtools files where it is flagging an error on functions that are too > complex. Could you find someone to fix those Patrick? Oh I see, we have configured the complexity rule as an error in our .eslintrc config file, but we have never configured a complexity threshold. And, according to https://github.com/eslint/eslint/issues/4808, doing this has no effect. So essentially, this rule was off for us. Now, since this change: https://github.com/eslint/eslint/commit/1288ba435d6758394bf97495583159b32742bfaa the default value is 20. And it looks like some of our functions are above this limit. Running eslint 2.7.0 locally produced the following results for me: \devtools\client\inspector\markup\markup.js 606:15 error Function '_onKeyDown' has a complexity of 32 complexity \devtools\client\netmonitor\netmonitor-view.js 1112:11 error Function 'sortBy' has a complexity of 21 complexity 1498:19 error Function '_flushRequests' has a complexity of 35 complexity 1781:30 error Function 'anonymous' has a complexity of 26 complexity So, because it was off until now, I would suggest changing our .eslintrc instead of fixing these functions (at least for now). This should do it: "complexity": [2, 35] I'll file a bug to reduce the threshold to 20 again and fix the functions.

https://reviewboard.mozilla.org/r/44619/#review41371 > is this part of another patch? or are you removing something that was unused to simplify the fix for receiveMessage below? Yeah this message was removed when the plugin-finder was dropped but this bit was never removed and the bit below made eslint fail.

https://reviewboard.mozilla.org/r/44623/#review41497 > We have the "avoid-escape" especially for this, so you could simply replace the backticks with single quotes instead: > > ``` > const HTML = '<div id="id" class="class" data-id="id"></div>'; > ``` Huh, looks like quotes considers using a single quote in that case as ok but not using backticks.

Comment on attachment 8731821 [details] MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40857/diff/2-3/

Comment on attachment 8731820 [details] MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40855/diff/2-3/

Comment on attachment 8738619 [details] MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44613/diff/1-2/

Comment on attachment 8738620 [details] MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44615/diff/1-2/

Comment on attachment 8738621 [details] MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44617/diff/1-2/

Comment on attachment 8738622 [details] MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44619/diff/1-2/

Comment on attachment 8731822 [details] MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40859/diff/2-3/

Comment on attachment 8738623 [details] MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44621/diff/1-2/

Comment on attachment 8738624 [details] MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44623/diff/1-2/

Comment on attachment 8731821 [details] MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps https://reviewboard.mozilla.org/r/40857/#review41735 I'm going to push back on adding semantic_version to the tree. Can we use LooseVersion from distutils (part of the standard library) instead? https://stackoverflow.com/questions/11887762/how-to-compare-version-style-strings

https://reviewboard.mozilla.org/r/40857/#review41735 I'm going to move the version checking stuff out to bug 1263211

Comment on attachment 8731821 [details] MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40857/diff/3-4/

Comment on attachment 8731820 [details] MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40855/diff/3-4/

Comment on attachment 8738619 [details] MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44613/diff/2-3/

Comment on attachment 8738620 [details] MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44615/diff/2-3/

Comment on attachment 8738621 [details] MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44617/diff/2-3/

Comment on attachment 8738622 [details] MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44619/diff/2-3/

Comment on attachment 8731822 [details] MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40859/diff/3-4/

Comment on attachment 8738623 [details] MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44621/diff/2-3/

Comment on attachment 8738624 [details] MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44623/diff/2-3/

Comment on attachment 8731821 [details] MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps https://reviewboard.mozilla.org/r/40857/#review41783 Looks good. +1 for pinning versions. I love more determinism!

https://hg.mozilla.org/integration/fx-team/rev/2b2647fba6ce https://hg.mozilla.org/integration/fx-team/rev/6cfdab897a8f https://hg.mozilla.org/integration/fx-team/rev/5bb356209841 https://hg.mozilla.org/integration/fx-team/rev/09680d0b42ac https://hg.mozilla.org/integration/fx-team/rev/4a108098012b https://hg.mozilla.org/integration/fx-team/rev/5e40adeb0332 https://hg.mozilla.org/integration/fx-team/rev/db2d6e47a3f4 https://hg.mozilla.org/integration/fx-team/rev/bf854efd8e65 https://hg.mozilla.org/integration/fx-team/rev/b332ba21d743

https://hg.mozilla.org/mozilla-central/rev/2b2647fba6ce https://hg.mozilla.org/mozilla-central/rev/6cfdab897a8f https://hg.mozilla.org/mozilla-central/rev/5bb356209841 https://hg.mozilla.org/mozilla-central/rev/09680d0b42ac https://hg.mozilla.org/mozilla-central/rev/4a108098012b https://hg.mozilla.org/mozilla-central/rev/5e40adeb0332 https://hg.mozilla.org/mozilla-central/rev/db2d6e47a3f4 https://hg.mozilla.org/mozilla-central/rev/bf854efd8e65 https://hg.mozilla.org/mozilla-central/rev/b332ba21d743