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

No useless expressions #690

Closed
dcousens opened this Issue Nov 18, 2016 · 29 comments

Comments

8 participants
@dcousens
Copy link
Member

dcousens commented Nov 18, 2016

I recently had something bite me in some of my own code, and I wonder if we can't avoid this happening:

var o = {}

// ...

if (o.value === undefined) {
  o.value === 3
}

That should have been o.value = 3, but, alas, something went wrong that day.
Is there an existing eslint rule for this?
Any reason not to have it enabled?

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 23, 2016

I would open an issue on ESLint about adding a rule for this, but I don't think it's possible for them to reliably detect that o.value === 3 is useless, since o.value could be a getter that mutates global state. Though, that's a terrible idea. :)

@feross feross closed this Nov 23, 2016

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented Nov 23, 2016

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 24, 2016

Nice find, going to re-open until we've released v9.

http://eslint.org/docs/rules/no-unused-expressions

@feross feross reopened this Nov 24, 2016

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 24, 2016

This rule has a lot of false positives with silly testing frameworks that do: expect(err).to.exist and other pointless sugary APIs.

# tests 422
# pass  401
# fail  21

People need to stop being so clever. All this fancy stuff just causes more problems than it solves.

How is this:

expect(err.message).to.equal('You had one job')

better than this?

assert.equal(err.message, 'You had one job')

Grrr...

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented Nov 24, 2016

Oh dear, does expect(err).to.exist really happen?
The grammar-like chaining (to.equal()) could make sense I suppose, and at least doesn't break any odd rules... but please not that ^

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 25, 2016

And guess what happens when you write expect(err).to.exists (note the extra s). It's undefined, so it silently does nothing.

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Nov 25, 2016

We could add this rule and encourage users to move away from that horrible api though?

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 25, 2016

Yeah, maybe we should just ship this. I don't want to scare too many people off of standard, but honestly, people need to use better modules.

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented Jan 3, 2017

What were your final thoughts @feross ?

@feross

This comment has been minimized.

Copy link
Member

feross commented Jan 7, 2017

I think we should ship this in the beta for v9 and see how it goes.

@dcousens dcousens added this to the standard v9 milestone Jan 24, 2017

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented Jan 24, 2017

@feross, I added this to the v9 milestone

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 9, 2017

Question: This rule has two options that relax parts of the rule. Should we enable them?

Without allowShortCircuit enabled, this is an error:

trySelectWire(false) || trySelectWire(true)

Without allowTernary enabled, this is an error:

isWebSeed ? piece.cancelRemaining(reservation) : piece.cancel(reservation)

More examples of both:

!valid ? reject(new HARError(validate.errors)) : resolve(data)
opts.alwaysOnTop ? emitBlur() : hideWindow()
oldRelease && oldRelease()

I would argue that all these should be if statements, since those are less clever. But if it's going to cause too much pain upgrading to v9, better to just allow these cases for now, so we can get the rest of the benefit of this rule.

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Feb 9, 2017

My vote is to enable, I use both of those patterns often. If I had to pick only one to enable I'd pick the ternary. I've flip-flopped on preference for if vs && a few times, no strong feelings either way, though I do agree that using if is a little less "clever". I think ecosystem impact will be revealing.

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 9, 2017

With no exceptions:

1..422
# tests 422
# pass  396
# fail  26

With both exceptions:

1..422
# tests 422
# pass  408
# fail  14

Of the 14 failures:

  • silly test framework (11)
  • bugs (3)

Here are the bugs:

if (Array.isArray(objects)) {
  return objects.some(function (object) { object.type && object.type !== type })
}
if (/use strict/.test(line)) line += '\n' + preamble
else line + preamble + '\n' + line
self.resetTimer
@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Feb 9, 2017

@feross anything interesting in the 12 that only passed with both exceptions enabled?

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 9, 2017

@timoxley No -- the 3 examples in my previous comment were representative (#690 (comment)).

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Feb 9, 2017

@feross i.e. no bugs getting through due to relaxed rule is good

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 9, 2017

For users of Chai, here's an explanation of why Chai's behavior is unsafe and an anti-pattern: Beware of libraries that assert on property access.

Fortunately, there is a workaround that doesn't require giving up Chai. Install dirty-chai, a project that "Extends Chai with lint-friendly terminating assertions."

In short, your tests will go from:

expect(true).to.be.true
foo.should.be.ok

To this:

expect(true).to.be.true()
foo.should.be.ok()

And just like that, your tests will be much more robust! (Many thanks to @erinspice for this tip.)


If you cannot change your tests now, feel free to continue using standard v8 until you're ready to upgrade.

If you wish to upgrade and temporarily disable this rule, add a comment to the top of each test file:

/* eslint-disable no-unused-expressions */
@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented Feb 9, 2017

if (Array.isArray(objects)) {
  return objects.some(function (object) { object.type && object.type !== type })
}

Haha, this gets me way too often.

Looking forward to it.

feross added a commit to standard/eslint-config-standard that referenced this issue Feb 9, 2017

Disallow Unused Expressions (no-unused-expressions)
Allow short circuiting and ternary expressions, since those are very
common but not ideal IMO.

Fixes: standard/standard#690
@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 9, 2017

This will be part of standard v9. I enabled both exceptions (short circuiting and ternary expressions), so it should only catch bugs and unsafe test frameworks.

@feross feross closed this Feb 9, 2017

feross added a commit to standard/eslint-config-standard that referenced this issue Feb 9, 2017

Disallow Unused Expressions (no-unused-expressions)
Allow short circuiting and ternary expressions, since those are very
common but not ideal IMO.

Fixes: standard/standard#690
@wmhilton

This comment has been minimized.

Copy link

wmhilton commented Feb 10, 2017

I love that one of the main decision factors for standard style is preventing buggy code. <3

@bcoe

This comment has been minimized.

Copy link

bcoe commented May 2, 2017

this rule breaks one of the common approaches that folks use when invoking yargs:

 yargs.argv

this would require that I need to update 100s of tests to continue using standard:

 /Users/benjamincoe/bcoe/yargs/test/command.js:33:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:78:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:104:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:129:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:509:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:527:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:549:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:586:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:602:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:619:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:713:5: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:719:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:729:5: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:792:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:835:5: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:850:7: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:873:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:884:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:896:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:907:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:918:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:930:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:944:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:955:9: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:972:13: Expected an assignment or function call and instead saw an expression.
  /Users/benjamincoe/bcoe/yargs/test/command.js:974:9: Expected an assignment or function call and instead saw an expression.

I'm less concerned about this, and more concerned that I'd need to update all of the existing documentation -- to point folks towards code that plays nice with standard.

I'm a huge advocate of standard, and have been using it for everything I write for the past couple years, but this rule specifically is making me a bit gun shy about upgrading.

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented May 2, 2017

@bcoe, but, you understand that yargs.argv, on its own, is completely weird... right?

@bcoe

This comment has been minimized.

Copy link

bcoe commented May 2, 2017

@dcousens .argv is the convention we've been using in optimist and yargs for years at this point, to indicate that we're done building up configuration, and the parser should run, e.g.,

#!/usr/bin/env node
var argv = require('yargs')
    .count('verbose')
    .alias('v', 'verbose')
    .argv

Just had yargs.argv as a minimal example -- I think there are probably literally thousands of repos using this approach in the wild, because it's what we've always documented in our examples.

@dcousens

This comment has been minimized.

Copy link
Member Author

dcousens commented May 2, 2017

If assigned as above, there is no issue.
It is the abuse of property side effects that is being discouraged; as it is completely unclear.

For standard users, you could promote a different approach, or, just add an ESLint ignore?

@bcoe

This comment has been minimized.

Copy link

bcoe commented May 2, 2017

@dcousens this comes up a lot when you're writing a CLI using yargs, e.g.:

require('yargs') // eslint-disable-line
  .usage('$0 <command>')
  .commandDir('../lib/commands')
  .help()
  .alias('h', 'help')
  .version()
  .strict()
  .demandCommand(1)
  .argv

or,

require('yargs') // eslint-disable-line
  .command(['serve', '*'], 'start the pkg-info-cache server', () => {}, (argv) => {
    const logger = bole(serviceName)
    bootstrapLogger()
    logger.info(argv)
    server(argv)
  })
  .option('port', {
    describe: 'port to run server on',
    default: process.env.PORT || 9999
  })
  .option('host', {
    describe: 'host to bind on',
    default: '0.0.0.0'
  })
  .argv

because yargs handles the flow control; one of the first things I do now when creating a bin in a project is to cargo cult the // eslint-disable-line this feels suboptimal though.

@knksmith57

This comment has been minimized.

Copy link

knksmith57 commented Sep 15, 2017

@bcoe fwiw just got bit hard by this; my google foo wasn't strong enough to find this thread (and not for lack of trying).

resorting to eslint-config-standard w/ all this boilerplate was the best I could do without forcing inline comments: https://gist.github.com/knksmith57/e433fa7416e8530427a4054cfa22b5d3

also a huge proponent of standard; literally forced it on every project I've ever had influence over.

have to say tho, requiring all that ^^ just to prevent a very idiomatic pattern from failing the build w/out becoming intrusive on teammates leaves a bad taste in my mouth. for all the time and debate standard has saved (and aims to save!) devs, this one was a big step back.

hope y'all reconsider your position on this one.

/cc @feross

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Sep 18, 2017

As of Yargs 9.0.0 this is no longer a problem 🎉 👏

Instead of using the getter .argv, you make a call to .parse().

  require('yargs')
    .command('hello <name>', 'say hello to <name>', {}, yargs => {
      console.log(`hello, ${yargs.name}!`)
    })
    .demandCommand(1)
    .help()
-   .argv
+   .parse()

/cc @knksmith57

kasbah added a commit to mcous/gerber-plotter that referenced this issue Apr 7, 2018

Nightpanda added a commit to Nightpanda/task-time-manager that referenced this issue May 4, 2018

Adds unit tests for resuming timer for a task
Adds dirty-chai plugin to chai. It help fix complains from
standardJS linting. It also makes the code more robust.
standard/standard#690 (comment)

Also adds sinon so we can test console.log outputs.

@feross feross added enhancement and removed enhancement labels May 10, 2018

@searls

This comment has been minimized.

Copy link

searls commented Jul 12, 2018

  1. Add Cypress to my project
  2. Get a boatload of these errors from the example tests it ships with
  3. Google to see what's up, find this thread
  4. Remember, "oh, right, chai is a thing and this is a bad API"
  5. Delete example tests

This is the system working, I think

@lock lock bot locked as resolved and limited conversation to collaborators Oct 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.