Update: Add ignoreDestructing option to camelcase rule (refs #9807) #10373

alunny · 2018-05-18T05:45:46Z

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

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

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).

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).

What changes did you make? (Give an overview)
The changes are in lib/rules/camelcase.js

Is there anything you'd like reviewers to focus on?
There are a couple of edge cases I mentioned on #9807, both of which are errors when ignore_destructuring: true is set in this change:

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;

g-plane · 2018-05-18T07:52:29Z

docs/rules/camelcase.md


-### always
+### Defaults (properties: "always", "ignoreDestructuring": false)


Should the docs about different options be separated into different sections?

I agree with this. Let's have sections for properties and ignoreDestructuring options separately, and create subsections for the always/never and true/false cases (respectively) for each.

platinumazure · 2018-05-20T13:06:22Z

Hi @alunny, thanks for contributing!

I don't have time to do a full review right now-- apologies-- but I do have two things that will need to change, so it's up to you if you want to fix them now or wait for a full review.

Commit message should end with "(fixes rule change proposal: destructured properties to be non-camelcased with camelcase rule #9807)" unless this change is only a partial fix
Commit message should start with "Update:" since we generally only use "New:" for completely new features (i.e., only if the rule itself is completely new)

Thanks again for the contribution, hopefully we can get this reviewed and (assuming all goes well) merged soon!

platinumazure

Hi @alunny, this generally looks good, just a few minor things.

Besides everything I've identified inline: Could we also add examples in documentation that are incorrect for ignoreDestructuring: true (i.e., the corner cases around renaming to snake case or creating a snake case rest property)?

Thanks for contributing, really looking forward to getting this in.

platinumazure · 2018-05-21T22:13:40Z

docs/rules/camelcase.md


-### always
+### Defaults (properties: "always", "ignoreDestructuring": false)


I agree with this. Let's have sections for properties and ignoreDestructuring options separately, and create subsections for the always/never and true/false cases (respectively) for each.

platinumazure · 2018-05-21T22:15:35Z

lib/rules/camelcase.js

+                return false;
+            }
+
+            return node.parent.type === "ObjectPattern" || isInsideObjectPattern(node.parent);


I don't strongly mind, but it would be nice to use iteration instead of recursion here.

platinumazure · 2018-05-21T22:17:19Z

tests/lib/rules/camelcase.js

@@ -292,12 +302,36 @@ ruleTester.run("camelcase", rule, {
            ]
        },
        {
-            code: "var { category_id: category_id } = query;",
+            code: "var { category_id: category_alias } = query;",


Should we have a test for the non-shorthand non-renaming case as well, to show that it should be valid? (I think it should be valid, since object-shorthand could report the redundancy of the property key/value name.)

Sounds good to me.

leipert · 2018-05-28T11:28:36Z

@alunny Are you working on this? This is a real pain-point for us, which this update would fix! I could also do the adjustments @platinumazure wanted you to make...

edit: thank you for working on this!

platinumazure · 2018-06-04T16:42:15Z

Friendly ping @alunny, are you still working on this? (If not, totally okay!)

If you're busy, would you be okay with someone pushing changes to your branch to help finish this off?

Thanks for all your work so far on this!

alunny · 2018-06-07T21:45:27Z

My bad @platinumazure and @leipert - was a bit busy these last few weeks.

I've blocked out some time today so should be able to push some changes.

alunny · 2018-06-07T22:14:46Z

lib/rules/camelcase.js

                        if (node.parent.shorthand && node.parent.value.left && isUnderscored(name)) {

                            report(node);
                        }

+                        const assignmentKeyEqualsValue = node.parent.key.name === node.parent.value.name;


hard to see in the GitHub ui since I clobbered the old commit, but in the previous rev this was:
const assignmentKeyEqualsValue = node.parent.key === node.parent.value;
which failed the {category_id: category_id} = query test

platinumazure

LGTM, thanks for contributing!

Left one suggestion in the docs, but this is not a blocker. (And great job with reorganizing the sections!)

platinumazure · 2018-06-07T22:35:08Z

docs/rules/camelcase.md

+var { category_id: category_id } = query;
+```
+
+Examples of **incorrect** code for this rule with the `{ "ignoreDestructuring": true }` option:


I would love to see this moved to being first in the section, but that's not a blocker for merging this. (Other rules tend to do incorrect than correct in most sections, but this is probably okay as is.)

Sounds good, will push that.

…9807)

kaicataldo · 2018-06-09T16:21:17Z

Thanks for contributing to ESLint!

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 18, 2018

g-plane reviewed May 18, 2018

View reviewed changes

platinumazure requested changes May 21, 2018

View reviewed changes

alunny force-pushed the issue9807/camelCase_destructuring branch from 19763c0 to 3ecd3e4 Compare June 7, 2018 22:13

alunny commented Jun 7, 2018

View reviewed changes

platinumazure approved these changes Jun 7, 2018

View reviewed changes

Update: Add ignoreDestructing option to camelcase rule (fixes eslint#…

99f2042

…9807)

alunny force-pushed the issue9807/camelCase_destructuring branch from 3ecd3e4 to 99f2042 Compare June 7, 2018 22:42

platinumazure changed the title ~~New: Add ignoreDestructing option to camelcase rule (refs #9807)~~ Update: Add ignoreDestructing option to camelcase rule (refs #9807) Jun 9, 2018

platinumazure requested a review from kaicataldo June 9, 2018 14:11

kaicataldo 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 Jun 9, 2018

kaicataldo approved these changes Jun 9, 2018

View reviewed changes

kaicataldo merged commit abf400d into eslint:master Jun 9, 2018

helixbass mentioned this pull request Aug 26, 2018

Fix: camelcase duplicate warning bug (fixes #10801) #10802

Merged

eslint-deprecated bot locked and limited conversation to collaborators Dec 8, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Add ignoreDestructing option to camelcase rule (refs #9807) #10373

Update: Add ignoreDestructing option to camelcase rule (refs #9807) #10373

alunny commented May 18, 2018

g-plane May 18, 2018

platinumazure May 21, 2018

platinumazure commented May 20, 2018

platinumazure left a comment

platinumazure May 21, 2018

platinumazure May 21, 2018

platinumazure May 21, 2018

alunny Jun 7, 2018

leipert commented May 28, 2018 •

edited

platinumazure commented Jun 4, 2018

alunny commented Jun 7, 2018 •

edited

alunny Jun 7, 2018

platinumazure left a comment

platinumazure Jun 7, 2018

alunny Jun 7, 2018

kaicataldo commented Jun 9, 2018


		### always
		### Defaults (properties: "always", "ignoreDestructuring": false)

Update: Add ignoreDestructing option to camelcase rule (refs #9807) #10373

Update: Add ignoreDestructing option to camelcase rule (refs #9807) #10373

Conversation

alunny commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented May 20, 2018

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leipert commented May 28, 2018 • edited

platinumazure commented Jun 4, 2018

alunny commented Jun 7, 2018 • edited

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Jun 9, 2018

leipert commented May 28, 2018 •

edited

alunny commented Jun 7, 2018 •

edited