-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Relaxed enforcement of camelcase rule #674
Conversation
@@ -21,7 +21,8 @@ eslintTester.addRuleTest("lib/rules/camelcase", { | |||
"myPrivateVariable_ = \"Patrick\"", | |||
"function doSomething(){}", | |||
"do_something()", | |||
"foo.do_something()" | |||
"foo.do_something()", | |||
"var foo = bar.baz_boom;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to read property values outside of a variable declaration:
if (bar.baz_zoom) {
// ...
}
This should also not trigger the warning.
@nzakas I pushed an update. Did I miss any cases? |
var PARENT_WHITELIST = [ | ||
"CallExpression", | ||
"VariableDeclarator", | ||
"IfStatement", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the wrong approach. The determining factor isn't the just the parent type. If it were, you'd need to list out a lot more node types here.
Really what we're looking at is anything that isn't a a CallExpression or AssignmentExpression, which can be found in any number of node types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's true. Take, for instance, your previous example:
if (bar.baz_boom) {
// ...
}
or
var foo = bar.baz_boom;
or
[foo.bar_baz]
These are all valid, but are not inside of a CallExpression
or AssignmentExpression
. I'd say that the determining factor is the parent, because the determining factor is the context in which the underscored Identifier
occurs. That's the exception to the rule that we're carving out.
It can never be the object on a MemberExpression
, but it can be the property on it, but only if that MemberExpression
is the right-hand of an AssignmentExpression
, the init of a VariableDeclarator
, an element of an ArrayExpression
, the test of an IfStatement
, or the direct child of a CallExpression
. The pattern seems to be that underscored properties are valid when they occur as a certain property on a node (test on IfStatement
, init on VariableDeclarator
, element on ArrayExpression
, etc.).
What I have written may not be the correct approach, but it does seem like the parent is what determines validity, unless I'm missing something or misunderstanding you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, I explained that poorly.
What I meant is that explicitly listing out all of the parents where it's
okay to use bar.baz_boom as a child isn't the correct approach - going down
that path we'll always miss one or two. You'd basically need to add every
possible place in the AST where a value could be read.
Really, the rules are pretty simple:
- Inside of a CallExpression, all is fair.
- Inside of an AssignmentExpression, the rightmost member of a
MemberExpression must be camelcase. - In all other cases, the identifier must be camel case.
I think the correct approach would be to address each of these
circumstances directly.
@nzakas I've pushed a different approach. I'm whitelisting underscored properties on |
] | ||
}, | ||
{ | ||
code: "var foo = { bar_baz: boom.bam_pow }", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests for nested properties?
Foo.bar_baz.name (should be valid)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the following be valid?
foo.bar_boom.qux = "value";
It is on the left-hand side of an assignment, but the use-case could easily be extending an API response that has underscores in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should be valid. The foo.bar_boom
is being read, the qux
is
being assigned.
The `camelcase` rule now allows reading underscored properties: var foo = bar.baz_boom; Fixes eslint#672
@nzakas Added some nested properties tests. |
LGTM. Thanks for your patience going through this. |
Relaxed enforcement of camelcase rule
Refactoring and learning is half the fun! |
The
camelcase
rule now allows reading underscored properties:Fixes #672