Fix: handle destructuring with defaults in camelcase rule (fixes #8511)

[vitorbal]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Fixes the bug reported in #8511. I changed the logic of the camelcase rule to properly handle destructuring with defaults.

Is there anything you'd like reviewers to focus on?
I think not!

[mention-bot]

@vitorbal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @not-an-aardvark and @gyandeeps to be potential reviewers.

[eslintbot]

LGTM

[not-an-aardvark]

Thanks for the PR!

It seems like the rule allows

/* eslint camelcase: [error, { properties: never }] */

var { foo: bar_baz } = qux;

Is this intentional? The bar_baz variable is not a property key, so it seems like the properties option shouldn't apply here.

That behavior also occurs without this PR, but I bring it up because this PR also makes it so that the following is valid, when it arguably shouldn't be:

var { foo: bar_baz = 1 } = quz;

[platinumazure]

Could you add a test where the default value is a variable declared elsewhere, where that variable is camelcase?

For example:

const my_default = 0;

function foo({ value_1 = my_default }) {}

I would expect my_default to be flagged in both locations as it's not a "property key".

[vitorbal]

Thanks for the quick feedback! Apologies on the delay.

@platinumazure: I don't think the rule checks the right-side of the default value assignment in the example you suggested if properties is set to never.
I'm not sure if this is intentional or not. Default values outside of destructuring are also handled the same way:

/* eslint camelcase: ["error", {properties: "never"}] */
const no_camelcased = 0;
function foo(value_1 = no_camelcased) {}

// ^ only reports the first occurrence of no_camelcased on line 1

[vitorbal]

Sorry all, I got caught up with work stuff. Just wanted to post an update since I haven't had much time for OSS the last few weeks.

@not-an-aardvark I agree with the destructuring rename issue you brought up. I've done some investigating and it looks like the code never took this into account, but I'm not sure if this was intentional or not.
I'm reworking the logic in the rule to take this into account now, and I think I'll have something ready by next weekend!

[kaicataldo]

@vitorbal Friendly ping - anything we can help with here?

[vitorbal]

@kaicataldo sorry, I've just been really busy with work stuff lately :(
I'm hoping to have a chance to pick up my slack on this during the weekend!

[vitorbal]

Super sorry, I've been quite busy at work and I won't have time to work on this for a while. Feel free to pick this up and finish it off if interested!

[erindepew]

I can pick this up :)

[ilyavolodin]

@erindepew Go for it! We appreciate any help!

[kaicataldo]

In regards to #8529 (comment) - could we land this with the fix for default params with destructuring and fix renaming destructured params in a separate PR?

Since they're two separate issues, it would be nice to avoid scope creep and land this!

[kaicataldo]

@platinumazure

I would expect my_default to be flagged in both locations as it's not a "property key".

I disagree on this one. The camelcase rule should only check when declaring/renamed destructing a variable, since that's the only time a user has control over the name.

import { my_default } from 'foo'; // This should warn
const { my_default } = require('foo'); // This should warn
const my_default = 0; // This should warn
const { my_default } = foo; // This should warn
const { bar: my_default } = foo; // This should warn
function foo({ value_1: my_default }) {} // This should warn
function foo({ value_1 = my_default }) {} // This should warn for `value_1` but not for `my_default`

The above errors would be fixed by the following:

import { my_default as myDefault } from 'foo';
const { my_default: myDefault } = require('foo');
const myDefault = 0;
const { my_default: myDefault } = foo;
const { bar: myDefault } = foo;
function foo({ value_1: myDefault }) {}
function foo({ value_1: myDefault = my_default }) {}

We can make a new issue for this to separate this discussion from the current PR - I'll try to get to it after lunch :) PR opened, is self-documenting: #9468

[kaicataldo]

Friendly ping @platinumazure @not-an-aardvark

[kaicataldo]

Re: #9468 I didn't realize that this all would touch the same part of the code. Maybe it would be easier to combine these into one PR as was originally suggested (and as is being done in the aforementioned PR)? Otherwise, we can get this merged and @erindepew can make a new PR once it lands. Either way, I'd love to keep this moving since we have a willing contributor!

[kaicataldo]

PR author has updated #9468 to include the changes from this PR as well as a bug fix for the destructuring reassignment bug! Shall we close this in favor of that PR now?

[platinumazure]

I'm okay with closing this if vitorbal is. Thanks to both of you for contributing, we really appreciate it.

[vitorbal]

I’m okay with closing this. Thanks guys for pushing it further, apologies again for going AWOL recently.

…

On Wed, Oct 18, 2017 at 3:43 AM Kevin Partington ***@***.***> wrote: I'm okay with closing this if vitorbal is. Thanks to both of you for contributing, we really appreciate it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmNdhkFI7IKzDU-bfL4yv4k0K3EzcNDks5stVfcgaJpZM4NMYNc> .

[kaicataldo]

@vitorbal No problem and no need to apologize! Thanks for your work on this. Closing in favor of #9468, which takes this PR and adds an additional bug fix.