Update: update handling of destructuring in camelcase (fixes #8511) #9468

erindepew · 2017-10-17T18:57:53Z

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 forked from #8529 and added a check for the right-side of the value assignment

Is there anything you'd like reviewers to focus on?
Nope!

eslintbot · 2017-10-17T18:57:54Z

LGTM

jsf-clabot · 2017-10-17T18:57:54Z

All committers have signed the CLA.

eslintbot · 2017-10-17T19:15:56Z

LGTM

eslintbot · 2017-10-17T19:16:46Z

LGTM

kaicataldo · 2017-10-17T20:10:46Z

I'm 👍 for this change - definitely seems like something the rule should catch.

I understand why this is branched off of the other PR, since it touches the same code, but I think the squashed commits will cause some issues with merging. Do we want to just combine the two PRs like was originally suggested, since they touch the same part of the code?
Thanks for updating this!

@vitorbal @platinumazure @not-an-aardvark Are you okay with closing #8529 in favor of this PR?

eslintbot · 2017-10-17T21:21:42Z

LGTM

platinumazure

LGTM, thanks! Do we want to include any of the new test cases in the documentation?

kaicataldo

Do you mind updating the PR title following the guidelines outlined here? This is important for us because we generate our changelogs and calculate the semver version of the release based on the commit messages in the commit history. Thanks!

kaicataldo · 2017-10-19T23:22:03Z

Marking as accepted since #8529 had already been accepted.

kaicataldo

Thanks for changing the PR title!

Have one comment, but I'd also love to see a test around destructuring with renaming and default values. Example (taken from #8529 (comment)):

var { foo: bar_baz = 1 } = quz;

It might also be nice to include a test for when multiple values are being destructured, just to ensure that works correctly.

kaicataldo · 2017-10-22T19:36:04Z

lib/rules/camelcase.js

+                } else if (node.parent.type === "Property" || node.parent.type === "AssignmentPattern") {
+
+                    if (node.parent.parent.type === "ObjectPattern") {
+                        node.parent.parent.properties.forEach(property => {


This method should worry about a single Property node at a time. I don't think we need to loop through all the properties here, as this method will already be called on each Identifier. The current implementation will loop through all the destructured properties for each destructured assignment that exists, which seems like more work than the rule needs to do.

kaicataldo · 2017-10-22T19:42:39Z

Requesting a review from @not-an-aardvark since you had concerns with the initial PR.

not-an-aardvark · 2017-10-22T21:22:54Z

I've been pretty busy lately, so I'll try to review this if I get the chance, but don't let me be a blocker if others are satisfied with the PR.

eslintbot · 2017-10-23T21:31:04Z

LGTM

eslintbot · 2017-10-23T21:40:57Z

LGTM

kaicataldo

This LGTM, but I would like to briefly discuss if this actually belongs behind the properties configuration option. I wonder if this should actually be on by default (as a semver-minor bug fix, which it technically already currently is) or if we should put it behind a destructuring configuration option. @erindepew @eslint/eslint-team Thoughts?

Apologies for thinking of this so late in the process. Also don't want to hold this PR up over a long discussion, but since this is a change that creates more errors I'd like to not have to do it twice :)

kaicataldo · 2017-10-26T17:12:10Z

TSC Summary: I'm bringing this to the TSC because of the complicated history of this PR - given the number of iterations and changes that have happened since the issue was originally accepted, I figured it would be easier to have the TSC decide.

Some history: The original PR - that this now includes - was accepted as a bug fix, and during the course of the original PR author working on it, another separate (but related) bug was discovered. It sounds like there is consensus to fix this bug, however, I'm concerned about putting it behind the properties option, since it doesn't really have anything to do with properties.

TSC Question: Should this be accepted as it currently is (behind the properties option), actually be on by default (as a semver-minor bug fix, which it technically already currently is) or put behind a destructuring configuration option to make it non-breaking?

kaicataldo · 2017-11-13T22:31:00Z

lib/rules/camelcase.js

-                        return;
+                    if (node.parent.parent && node.parent.parent.type === "ObjectPattern") {
+
+                        if (node.parent.key === node && node.parent.value !== node) {


I think it might be helpful to have a short comment that explains what this is checking.

kaicataldo · 2017-11-13T22:31:03Z

lib/rules/camelcase.js

+                            return;
+                        }
+
+                        if (node.parent.value.right && node.parent.value.right.type === "Identifier" && isUnderscored(node.parent.value.right.name)) {


I think it might be helpful to have a short comment that explains what this is checking.

kaicataldo · 2017-11-13T22:31:10Z

lib/rules/camelcase.js

+                            report(node);
+                        }
+
+                        if (node.parent.value.name && isUnderscored(node.parent.value.name)) {


I think it might be helpful to have a short comment that explains what this is checking.

kaicataldo · 2017-11-13T22:34:16Z

docs/rules/camelcase.md

+    // ...
+};
+
+function foo({ no_camelcased: my_default }) {


I wonder if this example would be clearer as something like:
function foo({ isCamelcased: no_camelcased }) {

kaicataldo · 2017-11-13T22:35:55Z

docs/rules/camelcase.md

@@ -60,6 +78,19 @@ var { category_id: category } = query;

 ### never

+
+Examples of **incorrect** code for this rule with the default `{ "properties": "never" }` option:


I don't think we need this or this change any more, since this behavior is the same as the cases listed above.

kaicataldo · 2017-11-13T22:38:40Z

tests/lib/rules/camelcase.js

@@ -79,6 +79,20 @@ ruleTester.run("camelcase", rule, {
        {
            code: "import { no_camelcased as camelCased, anoterCamelCased } from \"external-module\";",
            parserOptions: { ecmaVersion: 6, sourceType: "module" }
+        },
+        {
+            code: "const { no_camelcased = false } = bar;",


Shouldn't this be failing?

kaicataldo · 2017-11-13T22:38:48Z

tests/lib/rules/camelcase.js

+            parserOptions: { ecmaVersion: 6 }
+        },
+        {
+            code: "function foo({ no_camelcased = 'default value' }) {};",


Shouldn't this be failing?

kaicataldo · 2017-11-13T22:39:40Z

tests/lib/rules/camelcase.js

+        },
+        {
+            code: "const { no_camelcased = false } = bar;",
+            options: [{ properties: "never" }],


I don't think we need the properties any more in most of these valid tests. Maybe one with properties: "always" and one with properties: "never" at the end of the test suite to ensure they don't affect the outcome?

kaicataldo · 2017-11-13T22:40:33Z

tests/lib/rules/camelcase.js

+            parserOptions: { ecmaVersion: 6 }
+        },
+        {
+            code: "function foo({ no_camelcased: camelCased }) {};",


I think we're missing some of the test cases outlined in the docs.

kaicataldo · 2017-11-13T22:42:48Z

tests/lib/rules/camelcase.js

+                    type: "Identifier"
+                },
+                {
+                    message: "Identifier 'no_camelcased' is not in camel case.",


I don't think we should be warning for the assignment of no_camelcased. In other words, I don't think we should be checking the right hand side of destructured assignments with default values since they're only being referenced and not defined. The rule should warn on the declaration (which it correctly does).

kaicataldo · 2017-12-03T23:26:38Z

Looks like this has been updated.

@eslint/eslint-team Can we get some other eyes on this? This is a semver-minor bug fix (it will result in more errors being reported by default).

platinumazure

Just one question about a comment, otherwise looks good to me. Thanks for contributing!

platinumazure · 2017-12-04T00:48:58Z

lib/rules/camelcase.js

                        return;
                    }

-                    if (isUnderscored(name) && !ALLOWED_PARENT_TYPES.has(effectiveParent.type)) {
+                    // check right hand side of assignment to prevent duplicate warnings


Should this say "Don't check right hand side of AssignmentExpression (etc.)"?

…nt#8511)

adding to docs, tests, and fixing logic

erindepew · 2017-12-04T16:20:20Z

@platinumazure you're right, updated the comment! and thanks @kaicataldo for holding my hand through the process :)

kaicataldo

LGTM. Thanks for sticking with us on this one! I know it took a while with it being an inherited issue and the discussions that surrounded the implementation.

kaicataldo · 2017-12-08T21:19:34Z

Thanks for contributing to ESLint!

rodrigok · 2017-12-11T16:15:07Z

Should it mark _id as wrong too?

Example:

const { _id, msg, rid } = this.props.actionMessage;

/home/circleci/repo/app/containers/MessageActions.js
  232:11  error  Identifier '_id' is not in camel case  camelcase

kaicataldo · 2017-12-11T16:24:33Z

@rodrigok Can you make a new issue for this? These kinds of comments don't get much visibility. Thanks!

rodrigok · 2017-12-11T16:36:07Z

@kaicataldo Thanks, here is the issue #9709

j-f1 · 2017-12-11T16:36:11Z

@rodrigok See #9700 for the issue reporting this, and #9701 for the PR fixing the problem.

rodrigok · 2017-12-11T16:38:05Z

@j-f1 Thanks, I'll reference and close my issue.

erindepew force-pushed the 8511-default-destructuring branch from 795b6d7 to c905181 Compare October 17, 2017 19:16

kaicataldo mentioned this pull request Oct 17, 2017

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

Closed

kaicataldo added bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Oct 17, 2017

erindepew force-pushed the 8511-default-destructuring branch from c905181 to b0c134e Compare October 17, 2017 21:21

platinumazure approved these changes Oct 18, 2017

View reviewed changes

kaicataldo suggested changes Oct 19, 2017

View reviewed changes

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 Oct 19, 2017

gyandeeps changed the title ~~8511 default destructuring~~ Fix: handle destructuring with defaults in camelcase rule (fixes #8511) Oct 19, 2017

kaicataldo suggested changes Oct 22, 2017

View reviewed changes

kaicataldo requested a review from not-an-aardvark October 22, 2017 19:42

erindepew force-pushed the 8511-default-destructuring branch from b0c134e to a30e043 Compare October 23, 2017 21:31

kaicataldo reviewed Oct 23, 2017

View reviewed changes

kaicataldo removed the request for review from not-an-aardvark October 23, 2017 21:52

kaicataldo added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Oct 26, 2017

kaicataldo removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Oct 26, 2017

erindepew force-pushed the 8511-default-destructuring branch from 75f251a to b84be4c Compare November 13, 2017 22:07

erindepew changed the title ~~Fix: handle destructuring with defaults in camelcase rule (fixes #8511)~~ Update:: handle destructuring with defaults in camelcase rule (fixes #8511) Nov 13, 2017

platinumazure changed the title ~~Update:: handle destructuring with defaults in camelcase rule (fixes #8511)~~ Update: handle destructuring with defaults in camelcase rule (fixes #8511) Nov 13, 2017

erindepew force-pushed the 8511-default-destructuring branch 2 times, most recently from fe35472 to 6b82e4a Compare November 13, 2017 22:29

kaicataldo suggested changes Nov 13, 2017

View reviewed changes

erindepew force-pushed the 8511-default-destructuring branch from 33c053d to 791d406 Compare November 30, 2017 20:31

kaicataldo added the enhancement This change enhances an existing feature of ESLint label Dec 3, 2017

platinumazure reviewed Dec 4, 2017

View reviewed changes

vitorbal and others added 2 commits December 4, 2017 11:18

Fix: handle destructuring with defaults in camelcase rule (fixes esli…

8d9ed81

…nt#8511)

Update: checking right-side of default value assignment

1a4e72f

adding to docs, tests, and fixing logic

erindepew force-pushed the 8511-default-destructuring branch from 791d406 to 1a4e72f Compare December 4, 2017 16:19

ilyavolodin approved these changes Dec 8, 2017

View reviewed changes

kaicataldo approved these changes Dec 8, 2017

View reviewed changes

kaicataldo changed the title ~~Update: handle destructuring with defaults in camelcase rule (fixes #8511)~~ Update: update handling of destructuring in camelcase (fixes #8511) Dec 8, 2017

kaicataldo merged commit 256481b into eslint:master Dec 8, 2017

platinumazure mentioned this pull request Dec 9, 2017

The camelcase rule does not allow leading/trailing underscores for variable names #9700

Closed

rye mentioned this pull request Dec 10, 2017

Update eslint to the latest version 🚀 StoDevX/AAO-React-Native#1970

Closed

platinumazure mentioned this pull request Dec 13, 2017

camelcase check for properties seems broken #9715

Closed

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

eslint-deprecated bot locked and limited conversation to collaborators Jun 7, 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 Jun 7, 2018

		@@ -60,6 +78,19 @@ var { category_id: category } = query;

		### never


		Examples of incorrect code for this rule with the default `{ "properties": "never" }` option:

Update: update handling of destructuring in camelcase (fixes #8511) #9468

Update: update handling of destructuring in camelcase (fixes #8511) #9468

Conversation

erindepew commented Oct 17, 2017 • edited by qfox

eslintbot commented Oct 17, 2017

jsf-clabot commented Oct 17, 2017 • edited

eslintbot commented Oct 17, 2017

eslintbot commented Oct 17, 2017

kaicataldo commented Oct 17, 2017 • edited

eslintbot commented Oct 17, 2017

platinumazure left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo commented Oct 19, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Oct 22, 2017 • edited

Choose a reason for hiding this comment

kaicataldo commented Oct 22, 2017

not-an-aardvark commented Oct 22, 2017

eslintbot commented Oct 23, 2017

eslintbot commented Oct 23, 2017

kaicataldo left a comment • edited

Choose a reason for hiding this comment

kaicataldo commented Oct 26, 2017

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

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

kaicataldo commented Dec 3, 2017 • edited

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erindepew commented Dec 4, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo commented Dec 8, 2017

rodrigok commented Dec 11, 2017

kaicataldo commented Dec 11, 2017

rodrigok commented Dec 11, 2017

j-f1 commented Dec 11, 2017

rodrigok commented Dec 11, 2017

erindepew commented Oct 17, 2017 •

edited by qfox

jsf-clabot commented Oct 17, 2017 •

edited

kaicataldo commented Oct 17, 2017 •

edited

kaicataldo Oct 22, 2017 •

edited

kaicataldo left a comment •

edited

kaicataldo commented Dec 3, 2017 •

edited