Rule Proposal: prefer-destructuring #6053

Closed
mysticatea opened this Issue May 3, 2016 · 27 comments

Projects

Done in JSCS Compatibility

@mysticatea
Member

From requireArrayDestructuring and requireObjectDestructuring.

This rule will warn MemberExpressions as the favor of destructuring assignments.

{
    "prefer-destructuring": ["error", {"object": true, "array": true}]
}

JSCS documents seem to say that this doesn't prefer nested destructuring.
How do we address nested destructuring?

@mysticatea mysticatea added this to the JSCS Compatibility milestone May 3, 2016
@nzakas
Member
nzakas commented Jul 21, 2016

@markelog @mikesherov @mdevils @hzoo @zxqfox can you provide some insights for @mysticatea?

@zxqfox
Member
zxqfox commented Jul 21, 2016 edited

Nested destructures are not so easy to read as they supposed to be:

var metadata = {
    title: "Scratchpad",
    translations: [
       {
           title: "JavaScript-Umgebung"
       }
    ]
};

var { title: englishTitle, translations: [{ title: localeTitle }] } = metadata;
// the same as
var englishTitle = metadata.title;
var localeTitle = metadata.translations[0].title;

Guess we just need to support the basic JSCS functionality to allow users to migrate.
But personally I'd say there should be an option to restrict or require nested ones.

@markelog
Member
markelog commented Jul 21, 2016 edited

JSCS documents seem to say that this doesn't prefer nested destructuring.
How do we address nested destructuring?

Where?

Anyhow we can do this in two steps - first implement basic functionality and if there would be a request for it deal with nested stuff, I also think it could be an option disabled by default.

Also none of use actually implemented these rules.

@seanpdoyle Maybe you would like to help us port these rules to ESLint?

@kaicataldo
Member

So it sounds like we have a few votes for feature parity with the JSCS equivalent rules with the possibility of adding nested checks in the future. Totally agree with @zxqfox and I think it makes sense to only go one level deep to start off with. Forcing nested destructuring seems like something most users wouldn't want on by default.

@platinumazure
Member

I think we can agree on one level for now, just so we have something for users.

All in favor? 👍 from me.

@nzakas
Member
nzakas commented Aug 3, 2016

We still need a champion to drive this forward.

@kaicataldo
Member
kaicataldo commented Aug 3, 2016 edited

I can champion this, though I'm currently also championing one more JSCS rule and will get to that one first. If anyone else is interested in championing and can get to it sooner, feel free to let me know!

@kaicataldo kaicataldo self-assigned this Aug 3, 2016
@kaicataldo
Member
kaicataldo commented Aug 14, 2016 edited

@eslint/eslint-team Need some supporters for this to move forward!

@vitorbal
Member

👍 from me!

@alberto alberto added accepted and removed evaluating labels Aug 14, 2016
@kaicataldo
Member

Will be starting on this in the next few days.

@hzoo hzoo referenced this issue in DockYard/ember-suave Sep 9, 2016
Closed

Migrate to ESLint plugin. #113

@elodszopos

Not to rush anyone, just interested in a status on this. Any news @kaicataldo ?

Would be awesome to have the possibility to enforce stopping the this.props.blabla flood.

@kaicataldo
Member

Hey, thanks for checking in - ended up being busier than I expected and haven't gotten to it yet. Finally have some free time this weekend and am hoping to start in on this, but if someone else has the bandwidth please feel free to take it on!

@valera-rozuvan

Hi all! I am really interested in this rule. Any updates? Thanks!

@ilyavolodin
Member

@valera-rozuvan This proposal has been accepted, so it's just waiting for implementation. If you would like to implement this rule, and have questions, @kaicataldo as champion should be able to guide you.

@valera-rozuvan

@kaicataldo Have you seen alexlafroscia/eslint-plugin-ember-suave? That project already has a custom rule for ESLint ember-suave/prefer-destructuring:

Ex:

{
  "rules": {
    "ember-suave/prefer-destructuring": ["error", {
      "array": true,
      "object": true
    }]
  }
}

Should that implementation be taken as a basis? @alexlafroscia, @hzoo any input from your side?

@kaicataldo
Member

Apologies for not getting around to starting this yet.

@valera-rozuvan Would you like to work on implementing this? It does indeed look like that plugin does what we're trying to achieve here.

@not-an-aardvark
Member

I'm also willing to work on implementing this if no one else is working on it.

@alexlafroscia
Contributor

@valera-rozuvan AFAIK my implementation there works -- a lot of people in the Ember community use that package and I've never seen issues with it.

@valera-rozuvan
valera-rozuvan commented Dec 9, 2016 edited

Do we need to duplicate this functionality in main ESLint? If it's already available as a separate plugin, maybe it's best to spend our effort on something else?

@kaicataldo
Member

@valera-rozuvan Good point. It was already accepted by the team, so it has been deemed fit for core. However, since it's a JSCS compatibility issue, might be worth discussing to see the team thinks. We didn't implement some other rules that exist in other plugins.

@alexlafroscia If we decide we'd still like this in core, would you be amenable to either making a PR/allowing us to copy over the rule into core (with your author attribution in the rule file)?

@alexlafroscia
Contributor

@kaicataldo sure, say the word and I'll make the PR.

@platinumazure
Member

@kaicataldo I think we made a promise to JSCS users that they could (eventually) switch to ESLint and get basically all the JSCS functionality. To me it feels like it would be unfair to those users to tell them "actually, you need to use these plugins as well". Makes the conversion harder than it needs to be. I think this should absolutely be ported to core (with proper attribution).

@kaicataldo
Member
kaicataldo commented Dec 9, 2016 edited

@platinumazure I'm with you on that. Just wanted to discuss because there was at least (something regarding jQuery) that we said we would just recommend the plugin for.

There might have been some other factors that went into that though (for instance, that it's for jQuery, which is most likely not general enough for core).

Let me make it clear: I'm 👍 for prefer-destructuring in core, just wanted to open it for discussion for at least a day or two to make sure we're on board.

@alexlafroscia
Contributor

I'm putting the PR together to add the rule I already created as part of the core rules.

@alexlafroscia alexlafroscia added a commit to alexlafroscia/eslint that referenced this issue Dec 11, 2016
@alexlafroscia alexlafroscia New: `prefer-destructuring` rule (fixes #6053) 7b0b3db
@kaicataldo
Member

@alexlafroscia Much appreciated!

@not-an-aardvark not-an-aardvark added a commit that closed this issue Dec 16, 2016
@alexlafroscia @not-an-aardvark alexlafroscia + not-an-aardvark New: `prefer-destructuring` rule (fixes #6053) (#7741)
* New: `prefer-destructuring` rule (fixes #6053)

* Address general, easy PR feedback

* Remove checks covered by `object-shorthand`

* Add support for AssignmentExpression in `prefer-destructuring`

* Add `requireRenaming` option to the `prefer-destructuring` rule

* More PR feedback

* Add missing semicolon to code example in docs

* Update documentation
27424cb
@valera-rozuvan

Thank you all!

@ndaidong ndaidong added a commit to greenglobal/eslint-config-ggc that referenced this issue Jan 7, 2017
@ndaidong ndaidong Disable rule prefer-destructuring
- This rule has been added since eslint v3.13.0 eslint/eslint#6053, it should not be required rule.
b56832b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment