no-unused-vars very forgiving

[szkrd]

Is it intentional that no-unused-vars are set only for variables, but not for arguments? I can live with it of course, just curious.

"no-unused-vars": 2 vs. "no-unused-vars": [2, { "args": "all" }]

[LinusU]

It usually clashes with the ugly way that express defines handles error handlers...

I brought it up here #216 (comment) and here xojs/xo#6

Although I would actually be in favour of making it more strict 👍

[szkrd]

Ah, now I see, thank you!

[LinusU]

It might be a good idea to open up discussion about this again, and to run thru the tests and see how many repos would be affected by changing this...

[dcousens]

@LinusU I use express. I feel this pain.

[LinusU]

Yeah, me too. But it's literally the only place I have this problem though, which makes it a bit annoying since it could catch other bugs...

[szkrd]

We are mostly a koa shop :) Though adding it manually to eslintrc feels kinda dirty.

[dcousens]

@LinusU the other major places I had this issue (superagent) were eventually lobbied enough to remove it, but I still feel its pain from time to time.

[feross]

I just opened an issue on express about deprecating their four-argument middleware in favor of an explicit app.error method: expressjs/express#2896

[feross]

Ran { "args": "after-used" } through the test suite:

1..427
# tests 427
# pass  322
# fail  105

[akivajgordon]

For me, a consequence of not being informed about unused params can cause tests to fail. As an example, I started with an asynchronous test:

it('should foo', (done) => {
  somethingAsync().then(() => {
    expect(...)
    done()
  })
})

But then, I refactored my test and it no longer needed to be async:

it('should foo', (done) => {
  doSomething()
  expect(...)
})

Oops! Now my test fails because I never called done(). Had the linter warned me about done being unused, I would have removed it and not been running an asynchronous test. In other words, the mere presence of the done argument (or lack thereof) actually changes the way the test behaves, so it would be nice if I was warned about it.

[yoshuawuyts]

I believe we used to lint for this, but then stopped doing that. Not sure why, I missed the conversation on that one

…

On Thu, Jan 5, 2017 at 11:49 PM Akiva Gordon ***@***.***> wrote: For me, a consequence of not being informed about unused params can cause tests to fail. As an example, I started with an asynchronous test: it('should foo', (done) => { somethingAsync().then(() => { expect(...) done() }) }) But then, I refactored my test and it no longer needed to be async: it('should foo', (done) => { doSomething() expect(...) }) Oops! Now my test fails because I never called done(). Had the linter warned me about done being unused, I would have removed it and not been running an asynchronous test. In other words, the mere presence of the done argument (or lack thereof) actually changes the way the test behaves, so it would be nice if I was warned about it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWlersSYUoPvZJR9dFFvX9aG1bAz6Zoks5rPXOGgaJpZM4HaRxU> .

[dcousens]

Indeed, I am bitten by this often.
The few times it still occurs in express, I use _ to indicate an unused variable.

[dcousens]

What about

"no-unused-vars": [2, { "vars": "all", "args": "all", "argsIgnorePattern": "^_$" }],

[feross]

@akivajgordon Mocha should not count the number of arguments to the function to determine whether it's async or sync. Fortunately, this kind of fanciness is going out of fashion: it's planned to be removed from express and was already removed from other libraries that do it, like superagent. It should have explicit methods for sync and async tests, IMO.

@dcousens Any idea what the ecosystem impact of that rule change would be?

@yoshuawuyts This was removed well over a year ago, I believe.

[LinusU]

It won't be a problem in the case of mocha though, since you always call the done callback somehow, if your test is asynchronous...

[feross]

Moving discussion to here: standard/eslint-config-standard#136