Closed Bug 1318089 Opened 3 years ago Closed 3 years ago

Turn on no-unused-vars rule for AOM code

Categories

(Toolkit :: Add-ons Manager, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(2 files)

This rule tends to catch a lot of errors. We should turn it on for all add-on manager code.
Comment on attachment 8811461 [details]
Bug 1318089: Turn on no-unused-vars ESLint rule for toolkit/mozapps/extensions.

https://reviewboard.mozilla.org/r/93540/#review93902

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js:1008
(Diff revision 1)
>    writeInstallRDFForExtension(regexpblock_3, profileDir);
>    setExtensionModifiedTime(getFileForAddon(profileDir, regexpblock_3.id), Date.now() + 10000);
>  
>    startupManager(false);
>  
> -  let [s1, s2, s3, s4, s5, h, r] = yield promiseAddonsByIDs(ADDON_IDS);
> +  let [s1, s2, s3, /* s4 */, /* s5 */, h, r] = yield promiseAddonsByIDs(ADDON_IDS);

I think you can leave the comments out (which is pretty idiomatic in ES6 when dealing with array holes) e.g.:

`let [s1, s2, s3, , , h, r]`

But if you like the commented version better that's fine w/ me.
Attachment #8811461 - Flags: review?(rhelmer) → review+
Comment on attachment 8811461 [details]
Bug 1318089: Turn on no-unused-vars ESLint rule for toolkit/mozapps/extensions.

https://reviewboard.mozilla.org/r/93540/#review93902

> I think you can leave the comments out (which is pretty idiomatic in ES6 when dealing with array holes) e.g.:
> 
> `let [s1, s2, s3, , , h, r]`
> 
> But if you like the commented version better that's fine w/ me.

I thought about it, but this code is cryptic enough without not knowing what the commented out variables are supposed to be...
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffe6c6137d8e
Turn on no-unused-vars ESLint rule for toolkit/mozapps/extensions. r=rhelmer
Apparently I pushed before the last XPCShell test group finished, and of course it turned up a failure.

Apparently this:

  let realAddonRepo = AMscope.AddonRepository;

was meant to trigger a lazy getter to prevent the next line from throwing, but did not have a comment to indicate it. I guess it needs to be changed to:

  // Trigger the lazy getter so we can assign a new value:
  void AMscope.AddonRepository;
Comment on attachment 8811927 [details]
Bug 1318089: Follow-up: Re-add a lazy getter trigger that looked like a no-op.

https://reviewboard.mozilla.org/r/93820/#review93966
Attachment #8811927 - Flags: review?(rhelmer) → review+
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/808a44ee14bd
Follow-up: Re-add a lazy getter trigger that looked like a no-op. r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/ffe6c6137d8e
https://hg.mozilla.org/mozilla-central/rev/808a44ee14bd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.