prefer-destructuring reports false positives with compound assignments #7881

Closed
fasttime opened this Issue Jan 7, 2017 · 9 comments

Projects

None yet

5 participants

@fasttime
Contributor
fasttime commented Jan 7, 2017

Tell us about your environment

  • ESLint Version: 3.13.0
  • Node Version: 7.3.0
  • npm Version: 4.0.5

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

{ "parserOptions": { "ecmaVersion": 6 } }

What did you do? Please include the actual source code causing the issue.

/* eslint prefer-destructuring: error */

let foo = 0;
foo += bar.foo;

What did you expect to happen?

No error, I guess.

What actually happened? Please include the actual, raw output from ESLint.

  4:1  error  Use object destructuring                  prefer-destructuring
@eslintbot eslintbot added the triage label Jan 7, 2017
@not-an-aardvark not-an-aardvark added accepted bug rule and removed triage labels Jan 7, 2017
@not-an-aardvark
Member

I can reproduce this, thanks for the report.

It's happening because the rule is treating the line the same as

let foo = bar.foo;

...which could be fixed to

let {foo} = bar;
@not-an-aardvark not-an-aardvark self-assigned this Jan 7, 2017
@not-an-aardvark not-an-aardvark added a commit that referenced this issue Jan 7, 2017
@not-an-aardvark not-an-aardvark Fix: prefer-destructuring reporting compound assignments (fixes #7881) 6c1a312
@thealjey
thealjey commented Jan 8, 2017

This rule should only test variable definitions, not all types of assignments.
For example, this line is also a false positive:

global.window = jsdom().defaultView;
@not-an-aardvark
Member

@thealjey Actually, that looks like it's working as intended -- you can replace that line with

({ defaultView: global.window } = jsdom());
@thealjey
thealjey commented Jan 8, 2017 edited

@not-an-aardvark Adding parentheses, seriously?
This rule has a big potential to cleanup the code in variable definitions and function arguments.
But instead you're advising to just overcome its shortcomings, to write longer and harder to read code, simply to make a linting rule pass.
Do you really like what you've written, do you think it's an improvement?

Also, I don't think this rule should complain about things like this:

const {param} = something.property;

because:

  • I might not need a property variable
  • there already is a destructuring assignment
@not-an-aardvark
Member

There certainly might be room for improvement in the rule's design -- I was just clarifying that the rule is currently working as intended for your example.

@thealjey
thealjey commented Jan 8, 2017 edited

this rule is a very good idea, big 👍 to its author
I would never have come up with something like this myself
it's just that as an average ESLint user I got used to everything always being off the charts awesome
you guys are spoiling us 😃

@kaicataldo
Member

@thealjey Feel free to write up a proposal and submit a new issue if you see something you think could be improved.

@thealjey
thealjey commented Jan 9, 2017 edited

@kaicataldo
I personally am not that big on helping people :) Creating and properly writing an issue just seems like too much work, sorry 😊

And my suggestion is already out there, it's pretty simple.
It goes like this:

This rule should completely ignore everything but variable definitions (var, const, let, function arguments, commonjs imports) and ignore things that are already a destructuring assignment.

That's it, at least I cannot think of any other valid use for this rule.

@kaicataldo
Member

@thealjey Thanks for your input.

@keylocation-bot keylocation-bot referenced this issue in singapore/lint-condo Jan 11, 2017
Merged

Update dependency eslint to version 3.13.1 #215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment