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

rule change proposal: destructured properties to be non-camelcased with camelcase rule #9807

Closed
alunny-stripe opened this issue Jan 5, 2018 · 9 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@alunny-stripe
Copy link

alunny-stripe commented Jan 5, 2018

Hi eslint team! I've filled out the questions from https://github.com/eslint/eslint/blob/master/templates/rule-change-proposal.md but please let me know if you'd like any more detail here. I'm happy to work on a PR myself if this sounds like a sensible approach.

What rule do you want to change?

The camelcase rule.

Does this change cause the rule to produce more or fewer warnings?

Fewer.

How will the change be implemented? (New option, new default behavior, etc.)?

A new option:

"camelcase": [ "error", { "ignoreDestructuring": false }]
"camelcase": [ "error", { "ignoreDestructuring": true }]

Please provide some example code that this change will affect:

const { foo_bar } = baz;

What does the rule currently do for this code?

The rule currently always fails for these expressions (with or without the properties: "never" config option set).

See related discussion in #3185 and #9715

What will the rule do after it's changed?

If the "ignoreDestructuring": true option is set, the line of code above should pass the rule (while the rule should still be enforced for other identifiers in the source).

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 5, 2018
@j-f1
Copy link
Contributor

j-f1 commented Jan 5, 2018

Why do you need this option? You can still do const { foo_bar: fooBar } = baz.

@alunny-stripe
Copy link
Author

Mostly, I think const { foo_bar: fooBar } = baz is harder to read and noisier.

One case that also comes up a lot in the app I work on is somebody picking off properties from object (perhaps from a remote api) in order to use rest destructuring - I don't think this code:

const {some_property, some_other_one, ...rest} = remoteObject;
// do something with rest

is improved by:

const {some_property: someProperty, some_other_one: someOtherOne, ...rest} = remoteObject;
// do something with rest

or

/* eslint-disable camelcase */
const {some_property, some_other_one, ...rest} = remoteObject;
/* eslint-enable camelcase */
// do something with rest

It's also useful for object shorthand, when you're taking some remote data that has snake cased property names and using that for a subsequent request - for example:

  getIframeURL() {
    const {title, some_other_param} = this.props;
    const queryParams = qs.stringify({
      title,
      some_other_param,
    });
    return `${IFRAME_URL}?${queryParams}`;
  }

without modifying the rule this becomes:

  getIframeURL() {
    const {title, some_other_param: someOtherParam} = this.props;
    const queryParams = qs.stringify({
      title,
      some_other_param: someOtherParam,
    });
    return `${IFRAME_URL}?${queryParams}`;
  }

which is just more typing, and no more readable or maintainable.

@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jan 5, 2018
@jameswritescode
Copy link

Sorry if I'm commenting on something a little old, just hoping to support the proposal.

Where I am we've got a API in Ruby that we're sticking with the usual style-guides using snake_case style in the responses.

This is a pretty common pattern we're having to follow right now in our react apps:

const { listing_id: listingID, author_id: authorID } = this.props;

// ...

someFunc({ listing_id: listingID, author_id: authorID });

An option like this would be useful for keeping that kind of noise down to a minimum :)

@platinumazure
Copy link
Member

I'm a bit confused by the proposed option values. What does "always" or "never" mean: allow always/never, or warn always/never? I would much prefer something like ignoreDestructuring: true (or false) to make it more clear exactly what the configuration means.

@alunny-stripe
Copy link
Author

alunny-stripe commented May 2, 2018

@platinumazure good call - I think when I wrote up the proposal I was cargo-culting from some other config option. ignoreDestructuring: true is much better.

Updated the original issue description to reflect that.

@ilyavolodin
Copy link
Member

I think I would be fine with introducing this option as we already ignore object properties, and destructuring is basically access to object properties.

@alunny
Copy link
Contributor

alunny commented May 11, 2018

A couple cases that don't have the most obvious way to go with ignoreDestructuring: true came up when working on the PR. I think both of these should be invalid when ignore destructuring is on:

Creating a new identifier from a destructured assignment

const { category_id: category_alias } = query;

Creating a non-camelcased identifier for rest props:

const { category_id, ...other_props } = query;

@platinumazure
Copy link
Member

I'll champion this. Hopefully we can get another 👍 from a team member soon, otherwise this should probably be closed.

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 21, 2018
@platinumazure
Copy link
Member

Marking as accepted. Thanks @ilyavolodin, @aladdin-add, and @kaicataldo for endorsing this!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 8, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants