1299984 - Enable eslint no-unused-vars for shipping pwmgr code

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1299984 - Enable eslint no-unused-vars for shipping pwmgr code.

I also fixed two test cases while I was looking at the results.

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

Comment 2

[Johann Hofmann [:johannh]]

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?

Comment 3

[Matthew N. [:MattN]]

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 4

[Matthew N. [:MattN]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76216/diff/1-2/

Comment 5

[Johann Hofmann [:johannh]]

(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

[Johann Hofmann [:johannh]]

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

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

Comment 7

[Matthew N. [:MattN]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a093339f5b61
Enable eslint no-unused-vars for shipping pwmgr code. r=johannh

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a093339f5b61