Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change rule no-unused-vars to include args option #136

Closed
wants to merge 1 commit into from

Conversation

justinkalland
Copy link
Contributor

Currently no-unused-vars uses the option "args": "none". This proposed change removes the args option (relying on the default after-used).

Before the change:

((one, two) => {})() // passes

After the change:

((one, two) => { return one })() // fails

((one, two) => { return two })() // passes

References:

@justinkalland
Copy link
Contributor Author

justinkalland commented Dec 20, 2018

Ecosystem has no impact by this change:

1..187
# tests 187
# pass  186
# fail  1

Note: The one failing test is make-error and is addressed in standard/standard-packages#25

Copy link

@DiegoRBaquero DiegoRBaquero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see this. Would be a major version

@justinkalland
Copy link
Contributor Author

Agreed @DiegoRBaquero, I would also like to see this. I was waiting until it was "approved" and then I could start working through helping the effected projects.

@feross
Copy link
Member

feross commented Jul 11, 2019

You can read why this was not enabled in the past: standard/standard#419

Basically, because of how express error route handlers work, if you remove unused arguments then can actually change an error router handler into a normal route handler. They explicitly look for functions with 4 arguments and treat those as error handlers. It's a bad design that they're still trying to figure out how to change.

So, if we enable this, then folks would need to be really careful not to accidentally remove the unused arguments at the suggestion of standard and break their express apps. It's a tricky one.

@feross feross closed this Oct 28, 2020
@feross
Copy link
Member

feross commented Oct 28, 2020

Moving discussion to standard/standard#419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unused variables don't output warnings
3 participants