Closed Bug 1440379 Opened 6 years ago Closed 6 years ago

Tidy up ESlint no-unused-vars definitions wrt Ci/Cu/Cr/Cc usage for varsIgnorePattern

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P1)

3 Branch
enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

no-unused-vars has various whitelists - generally for `Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS`.

We can now remove the C* ones, but we're not quite ready for the EXPORTED_SYMBOLS yet (due to sorting out javascript module files).

Since removing the C* ones actually uncovers a few issues (the default in recommended.js was wrong!), landing this earlier seems better.
Assignee: nobody → standard8
Priority: P3 → P1
Comment on attachment 8953130 [details]
Bug 1440379 - Tidy up ESlint no-unused-vars definitions wrt Ci/Cu/Cr/Cc usage for varsIgnorePattern.

https://reviewboard.mozilla.org/r/222400/#review229018

::: .eslintrc.js:65
(Diff revision 3)
>      "rules": {
>        "mozilla/mark-exported-symbols-as-used": "error",
>        "no-unused-vars": ["error", {
>          "args": "none",
>          "vars": "local",
> -        "varsIgnorePattern": "^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS"
> +        "varsIgnorePattern": "^EXPORTED_SYMBOLS"

Did you mean to add a $ here?

::: services/sync/tps/extensions/tps/resource/tps.jsm:11
(Diff revision 3)
>    * Components.utils.import() and acts as a singleton. Only the following
>    * listed symbols will exposed on import, and only when and where imported.
>    */
>  
>  var EXPORTED_SYMBOLS = [
> -  "ACTIONS", "Addons", "Addresses", "Bookmarks",
> +  "ACTIONS", "Addons", "Addresses", "Bookmarks", "CreditCards",

How is tools/lint/eslint/modules.json getting updated?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js:357
(Diff revision 3)
>      "no-unsanitized/property": "error",
>  
>      // No declaring variables that are never used
>      "no-unused-vars": ["error", {
>        "args": "none",
>        "vars": "local",

How much work is left to enable '"vars": "all"' here?
Comment on attachment 8953130 [details]
Bug 1440379 - Tidy up ESlint no-unused-vars definitions wrt Ci/Cu/Cr/Cc usage for varsIgnorePattern.

https://reviewboard.mozilla.org/r/222400/#review229018

> How much work is left to enable '"vars": "all"' here?

Lots of work unfortunately, and I don't see it is viable any time soon. The main issue is our content/ code and the way we load it into windows - multiple files pick up globals from multiple other files, and then share some back as well (circular dependencies, yay!). A lot of the time it isn't clear what is/isn't exported, and also which files get to use other files (as that's hidden in xul/html files). IMHO it is one of the biggest messes we have.

To sort it out, we'd have to create export lists for each js file, and then have people maintain them properly - and for the real benefit I think we'd need to sort out how the files are loaded/included. Due to the way our code is currently structured, I don't see that as really viable any time soon, and I'd rather push for us reworking the architecture somehow.
Comment on attachment 8953130 [details]
Bug 1440379 - Tidy up ESlint no-unused-vars definitions wrt Ci/Cu/Cr/Cc usage for varsIgnorePattern.

https://reviewboard.mozilla.org/r/222400/#review229064

Looks good, thanks!
Attachment #8953130 - Flags: review?(florian) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0dfc8b693111
Tidy up ESlint no-unused-vars definitions wrt Ci/Cu/Cr/Cc usage for varsIgnorePattern. r=florian
https://hg.mozilla.org/mozilla-central/rev/0dfc8b693111
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.