Closed Bug 1249627 Opened 4 years ago Closed 4 years ago

Eslint: allow unused arguments

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a follow up for bug 1247434.

Let's update the .eslintrc file to allow not used function arguments. In many cases it's useful to see the arguments event if they are not used (e.g. the 'event' argument) and there is no risk.

Honza
Attached patch bug1249627.patch (obsolete) — Splinter Review
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8721317 - Flags: review?(pbrosset)
Comment on attachment 8721317 [details] [diff] [review]
bug1249627.patch

Review of attachment 8721317 [details] [diff] [review]:
-----------------------------------------------------------------

I'm personally fine with this change, since I suggested it in the first place.
But it might be a good idea to ask other people to make sure we're not taking this decision in isolation and have other people disagree and change it back :)
Maybe let's check it in in fx-team for now and send an email to dev-developer-tools to ask if anyone is against the change. It's easy to revert if needed.

::: devtools/.eslintrc
@@ +264,5 @@
>      "no-unneeded-ternary": 2,
>      // Disallow unreachable statements after a return, throw, continue, or break
>      // statement.
>      "no-unreachable": 2,
>      // Disallow declaration of variables that are not used in the code

Maybe update this comment to something like:

// Disallow global and local variables that aren't used, but allow unused function arguments.
Attachment #8721317 - Flags: review?(pbrosset) → review+
Attached patch bug1249627.patchSplinter Review
Comment updated
Honza
Attachment #8721317 - Attachment is obsolete: true
Attachment #8721323 - Flags: review+
A message sent to our public newsgroup.
Honza
Keywords: checkin-needed
I feel we should keep the original eslint to disallow unused arguments until we can be confident our devtools are in a good eslint state. There are genuine cases where an unused argument should be removed and having this eslint warning would be helpful with that.
I was wondering if it would be possible to have a naming convention, where an
unused argument would have to be spelled a certain way (leading or trailing underscore,
something like that); and then eslint would additionally ensure that the argument
was truly unused.
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #5)
> I feel we should keep the original eslint to disallow unused arguments until
> we can be confident our devtools are in a good eslint state. There are
> genuine cases where an unused argument should be removed
Do you have any examples?

Honza
Flags: needinfo?(gl)
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #5)
> > I feel we should keep the original eslint to disallow unused arguments until
> > we can be confident our devtools are in a good eslint state. There are
> > genuine cases where an unused argument should be removed
> Do you have any examples?
> 
> Honza

I only recall fixing an instance of this while doing some eslint cleanups. I suspect this happened because of some changes to a function and an argument is no longer need.
Flags: needinfo?(gl)
Patrick, can you please make a decision here?
This little thing is blocking big pile of my patches.

Honza
Flags: needinfo?(pbrosset)
My take on this is that if there are genuine cases where this rule helps avoid errors, they are a minority.
My experience with this rule is that it is way more often annoying than helpful (I'm only talking about checking for unused arguments here, we def. need to continue checking for unused global and local variables).

I did some research in devtools/client/ (not devtools/ to avoid having too much data to deal with, but still have a representative code base), and we have 491 instances of unused arguments there.
Test files are important too, but I decided to focus on non-test files only, to reduce the data further. So, in non-test files, we have 178 unused arguments. 52 of which are unused event arguments in event handler callbacks.
Looking quickly through the results, I can roughly estimate that ~80% of unused arguments are default arguments that we *want* to keep in a function signature: events in event handlers, require/exports/module in browser modules, default arguments in react components, default arguments in redux actions/reducers, default arguments in idl implementations, etc...

So, I do agree that some of them might be confusing and should be removed, but I think the vast majority is harmless and, at times, even wanted. I also think that there are too many variants for us to create a rule that would check this automatically.

So I'm in favor of not checking for unused arguments anymore.
This patch is R+ and checkin-needed, let's keep it this way.
Flags: needinfo?(pbrosset)
Whoops, sorry, Wanted to push this commit myself but the tree is closed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b795629bce7f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.