Enable eslint no-unused-vars for shipping pwmgr code

RESOLVED FIXED in Firefox 51

Status

()

--
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [passwords:tech-debt])

Attachments

(1 attachment)

Cleanup unused variables in the code and enable the no-unused-vars (excluding arguments) for our shipping code (excluding the test directory).
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8787466 [details]
Bug 1299984 - Enable eslint no-unused-vars for shipping pwmgr code.

https://reviewboard.mozilla.org/r/76216/#review74352

::: toolkit/components/passwordmgr/.eslintrc:14
(Diff revision 1)
>  
>      // Commas at the end of the line not the start
>      "comma-style": 2,
> +
> +    // Don't allow unused local variables unless they match the pattern
> +    "no-unused-vars": [2, {"args": "none", "vars": "local", "varsIgnorePattern": "^(C[ciru]|ids|ignored|unused)$"}],

I'm not too happy that we're ignoring all unused variables named 'ids' to deal with the destructuring here:

http://searchfox.org/mozilla-central/search?q=%5Blogins%2C+ids%5D&case=false&regexp=false&path= 

It's unfortunate that eslint doesn't allow for ignoring values used in tuple-like array destructuring.

An alternative solution would be to ignore variables starting with an _underscore and rename ids to _ids where it's not used. This is the prevalent style in most functional languages afaik.

Also, I'm not sure C[ciru] is needed. Aren't these global variables?
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8787466 [details]
Bug 1299984 - Enable eslint no-unused-vars for shipping pwmgr code.

https://reviewboard.mozilla.org/r/76216/#review74352

> I'm not too happy that we're ignoring all unused variables named 'ids' to deal with the destructuring here:
> 
> http://searchfox.org/mozilla-central/search?q=%5Blogins%2C+ids%5D&case=false&regexp=false&path= 
> 
> It's unfortunate that eslint doesn't allow for ignoring values used in tuple-like array destructuring.
> 
> An alternative solution would be to ignore variables starting with an _underscore and rename ids to _ids where it's not used. This is the prevalent style in most functional languages afaik.
> 
> Also, I'm not sure C[ciru] is needed. Aren't these global variables?

(In reply to Johann Hofmann [:johannh] from comment #2)
> Comment on attachment 8787466 [details]
> Bug 1299984 - Enable eslint no-unused-vars for shipping pwmgr code.
> 
> https://reviewboard.mozilla.org/r/76216/#review74352
> 
> ::: toolkit/components/passwordmgr/.eslintrc:14
> (Diff revision 1)
> >  
> >      // Commas at the end of the line not the start
> >      "comma-style": 2,
> > +
> > +    // Don't allow unused local variables unless they match the pattern
> > +    "no-unused-vars": [2, {"args": "none", "vars": "local", "varsIgnorePattern": "^(C[ciru]|ids|ignored|unused)$"}],
> 
> I'm not too happy that we're ignoring all unused variables named 'ids' to
> deal with the destructuring here:
> 
> http://searchfox.org/mozilla-central/
> search?q=%5Blogins%2C+ids%5D&case=false&regexp=false&path= 
> 
> It's unfortunate that eslint doesn't allow for ignoring values used in
> tuple-like array destructuring.
> 
> An alternative solution would be to ignore variables starting with an
> _underscore and rename ids to _ids where it's not used. This is the
> prevalent style in most functional languages afaik.

I would just delete the argument in the array if I wanted to change blame (which I was trying to avoid hence the ignoring). Renaming would both change blame and still need an ignore rule so it seems like the worst of both worlds IMO. I also am not really familiar with or don't follow the convention of using a leading underscore for unused variables. Why wouldn't they just get rid of the unused variable?

> Also, I'm not sure C[ciru] is needed. Aren't these global variables?

This was necessary before I decided to disable the rule in the test directory as sometimes they don't look like globals but will become globals once loaded in another scope: https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/toolkit/components/passwordmgr/test/pwmgr_common.js#418

I can remove it now though.
Comment hidden (mozreview-request)
(In reply to Matthew N. [:MattN] from comment #3)
> I would just delete the argument in the array if I wanted to change blame
> (which I was trying to avoid hence the ignoring). Renaming would both change
> blame and still need an ignore rule so it seems like the worst of both
> worlds IMO. I also am not really familiar with or don't follow the
> convention of using a leading underscore for unused variables. Why wouldn't
> they just get rid of the unused variable?

I was thinking of languages that have a distinct tuple type (like most ML derived langs), requiring you to match the tuple length when destructuring. I didn't realize this was to preserve blame. :)

I guess there isn't a perfect way to handle this, but I'd prefer changing the blame and removing the ids. I think subfolders should be conservative with adding exceptions.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8787466 [details]
Bug 1299984 - Enable eslint no-unused-vars for shipping pwmgr code.

https://reviewboard.mozilla.org/r/76216/#review74728
Attachment #8787466 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #5)
> I guess there isn't a perfect way to handle this, but I'd prefer changing
> the blame and removing the ids.

Since this is only for no-unused-vars where there is often no actual harm in unused variables and the variable name `ids` isn't a very common name outside this component, I would rather leave it as-is for now.

> I think subfolders should be conservative with adding exceptions.

If I understand you correctly then I think I disagree as I would rather we be more aggressive about enabling eslint rules in general, especially for new code, rather than waiting for everything to pass before enabling a rule.

Comment 8

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a093339f5b61
Enable eslint no-unused-vars for shipping pwmgr code. r=johannh

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a093339f5b61
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.