Fix: camelcase rule fix for import declarations (fixes #6755) #6784

lordgiotto · 2016-07-28T08:09:33Z

Calmecase rule now doesn't report an error if camelcased identifier is imported with a camecased alias. Otherwhise it reports an error as expected.

mention-bot · 2016-07-28T08:09:34Z

@lordgiotto, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @nzakas and @iancmyers to be potential reviewers

eslintbot · 2016-07-28T08:09:35Z

LGTM

vitorbal · 2016-07-28T08:14:33Z

tests/lib/rules/camelcase.js

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


Could you add a test for import { no_camelcased as no_camel_cased } from \"external module\"; as well?

Ok, I added that test and slightly changed the rule check to not report the error twice: now it only checks local identifier (that is the one we care about) and never the imported one.

@vitorbal Added both the tests you suggested :)

thanks, looking great!

eslintbot · 2016-07-28T08:40:52Z

LGTM

nzakas · 2016-07-28T15:58:00Z

LGTM, just waiting another day to let others review.

vitorbal · 2016-07-29T08:48:39Z

tests/lib/rules/camelcase.js

+        },
+        {
+            code: "import { no_camelcased as camelCased, anoterCamelCased } from \"external-module\";",
+            parserOptions: { ecmaVersion: 6, sourceType: "module" }


Sorry for not asking this before, could you also add a test for import { camelCased } from \"external module\"; as well, just in case?

eslintbot · 2016-07-29T09:14:41Z

LGTM

eslintbot · 2016-07-29T09:33:51Z

LGTM

vitorbal · 2016-07-29T09:40:57Z

LGTM

kaicataldo · 2016-07-29T17:53:49Z

Thanks for contributing, @lordgiotto! Code changes looks great. Can you add a description of this behavior to the docs for the rule?

eslintbot · 2016-07-30T22:15:37Z

LGTM

lordgiotto · 2016-07-30T22:20:55Z

@kaicataldo Docs updated! Please, let me know if what I wrote is acceptable: since english is not my mother thong, even it's good enough to communicate, it could be not good enough to write official documentation :P

kaicataldo · 2016-07-31T23:20:18Z

docs/rules/camelcase.md

@@ -4,7 +4,7 @@ When it comes to naming variables, style guides generally fall into one of two c

 ## Rule Details

-This rule looks for any underscores (`_`) located within the source code. It ignores leading and trailing underscores and only checks those in the middle of a variable name. If ESLint decides that the variable is a constant (all uppercase), then no warning will be thrown. Otherwise, a warning will be thrown. This rule only flags definitions and assignments but not function calls.
+This rule looks for any underscores (`_`) located within the source code. It ignores leading and trailing underscores and only checks those in the middle of a variable name. If ESLint decides that the variable is a constant (all uppercase), then no warning will be thrown. Otherwise, a warning will be thrown. This rule only flags definitions and assignments but not function calls. In case of es6 `import` statements, this rule only targets the name of the variable that will be imported into the local module scope.


This looks great to me! Could we make it ES6 instead of es6 for consistency with our other documentation?

Fixed, thank you :)

kaicataldo · 2016-07-31T23:21:46Z

@lordgiotto Your English is great. One tiny, tiny nitpick, but otherwise the docs look really good to me. Thanks!

eslintbot · 2016-08-01T10:05:27Z

LGTM

kaicataldo · 2016-08-01T14:33:01Z

Looks like the tests are failing, but doesn't seem like it's because of anything in this PR:

Validating licenses
undefined license for circular-json@0.3.0 is impermissible.
npm ERR! Test failed.  See above for more details.

gyandeeps · 2016-08-01T14:48:39Z

huh, I didn't know that we check for license on indirect dependencies also. I always thought we only checked for direct dependencies.

lordgiotto · 2016-08-01T15:13:27Z

Very strange, this is my output of npm test: no licenses errors.
eslint-test.txt

nzakas · 2016-08-01T16:05:50Z

Hmm, can someone track down where that dependency is building pulled in?

Also, note for whomever ends up merging: The commit message should begin with "Update:" since it adds new default functionality.

lordgiotto · 2016-08-01T16:39:54Z

@nzakas ok, i'll change commit message, I misunderstood the guide and used 'fix' since it was referred to an issue marked as bug, sorry.

Calmecase rule now doesn't report an error if camelcased identifier is imported with a camecased alias. Otherwhise it reports an error as expected.

eslintbot · 2016-08-01T18:21:54Z

LGTM

lordgiotto · 2016-08-01T19:08:04Z

@kaicataldo I pushed a new commit with the correct message and this time it passed all the test. Weird :)

platinumazure · 2016-08-01T19:38:30Z

@lordgiotto We managed to get a PR into the upstream project that was causing the test failures, and now everything is fixed. 😄 Thanks for your patience!

vitorbal · 2016-08-01T20:05:42Z

@lordgiotto thanks a lot for your contribution! We really appreciate it 🎉

lordgiotto · 2016-08-02T07:03:25Z

Thank you @vitorbal and all the reviewers, it was a pleasure :)

jquerybot added the CLA: Valid label Jul 28, 2016

vitorbal reviewed Jul 28, 2016
View reviewed changes

lordgiotto force-pushed the issue6755 branch from ebfc532 to a5382f9 Compare July 28, 2016 08:40

vitorbal reviewed Jul 29, 2016
View reviewed changes

lordgiotto force-pushed the issue6755 branch from a5382f9 to c435bb1 Compare July 29, 2016 09:14

lordgiotto force-pushed the issue6755 branch from c435bb1 to 426ea12 Compare July 29, 2016 09:33

lordgiotto force-pushed the issue6755 branch from 426ea12 to 26774d1 Compare July 30, 2016 22:15

kaicataldo reviewed Jul 31, 2016
View reviewed changes

lordgiotto force-pushed the issue6755 branch from 26774d1 to 93ad733 Compare August 1, 2016 10:05

Update: camelcase rule fix for import declarations (fixes eslint#6755)

1d80568

Calmecase rule now doesn't report an error if camelcased identifier is imported with a camecased alias. Otherwhise it reports an error as expected.

lordgiotto force-pushed the issue6755 branch from 93ad733 to 1d80568 Compare August 1, 2016 18:21

gyandeeps merged commit e524d16 into eslint:master Aug 1, 2016

lordgiotto deleted the issue6755 branch August 4, 2016 13:46

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: camelcase rule fix for import declarations (fixes #6755) #6784

Fix: camelcase rule fix for import declarations (fixes #6755) #6784

lordgiotto commented Jul 28, 2016

mention-bot commented Jul 28, 2016

eslintbot commented Jul 28, 2016

vitorbal Jul 28, 2016

lordgiotto Jul 28, 2016

lordgiotto Jul 29, 2016

vitorbal Jul 29, 2016

eslintbot commented Jul 28, 2016

nzakas commented Jul 28, 2016

vitorbal Jul 29, 2016

eslintbot commented Jul 29, 2016

eslintbot commented Jul 29, 2016

vitorbal commented Jul 29, 2016 •

edited

kaicataldo commented Jul 29, 2016

eslintbot commented Jul 30, 2016

lordgiotto commented Jul 30, 2016

kaicataldo Jul 31, 2016 •

edited

lordgiotto Aug 1, 2016

kaicataldo commented Jul 31, 2016

eslintbot commented Aug 1, 2016

kaicataldo commented Aug 1, 2016

gyandeeps commented Aug 1, 2016

lordgiotto commented Aug 1, 2016 •

edited

nzakas commented Aug 1, 2016

lordgiotto commented Aug 1, 2016

eslintbot commented Aug 1, 2016

lordgiotto commented Aug 1, 2016

platinumazure commented Aug 1, 2016

vitorbal commented Aug 1, 2016

lordgiotto commented Aug 2, 2016

Fix: camelcase rule fix for import declarations (fixes #6755) #6784

Fix: camelcase rule fix for import declarations (fixes #6755) #6784

Conversation

lordgiotto commented Jul 28, 2016

mention-bot commented Jul 28, 2016

eslintbot commented Jul 28, 2016

vitorbal Jul 28, 2016

Choose a reason for hiding this comment

lordgiotto Jul 28, 2016

Choose a reason for hiding this comment

lordgiotto Jul 29, 2016

Choose a reason for hiding this comment

vitorbal Jul 29, 2016

Choose a reason for hiding this comment

eslintbot commented Jul 28, 2016

nzakas commented Jul 28, 2016

vitorbal Jul 29, 2016

Choose a reason for hiding this comment

eslintbot commented Jul 29, 2016

eslintbot commented Jul 29, 2016

vitorbal commented Jul 29, 2016 • edited

kaicataldo commented Jul 29, 2016

eslintbot commented Jul 30, 2016

lordgiotto commented Jul 30, 2016

kaicataldo Jul 31, 2016 • edited

Choose a reason for hiding this comment

lordgiotto Aug 1, 2016

Choose a reason for hiding this comment

kaicataldo commented Jul 31, 2016

eslintbot commented Aug 1, 2016

kaicataldo commented Aug 1, 2016

gyandeeps commented Aug 1, 2016

lordgiotto commented Aug 1, 2016 • edited

nzakas commented Aug 1, 2016

lordgiotto commented Aug 1, 2016

eslintbot commented Aug 1, 2016

lordgiotto commented Aug 1, 2016

platinumazure commented Aug 1, 2016

vitorbal commented Aug 1, 2016

lordgiotto commented Aug 2, 2016

vitorbal commented Jul 29, 2016 •

edited

kaicataldo Jul 31, 2016 •

edited

lordgiotto commented Aug 1, 2016 •

edited