Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update: check renaming identifier in object destructuring (fixes 12827) #12881

Merged
merged 11 commits into from Feb 17, 2020
Merged

Update: check renaming identifier in object destructuring (fixes 12827) #12881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4ad9e98
Update: check renaming identifier in object destructuring (fixes 12827)
yeonjuan Feb 6, 2020
213b236
Fix wrong error type
yeonjuan Feb 6, 2020
ce40f92
ignore properties options on object pattern, ignore computed prop
yeonjuan Feb 7, 2020
6c235de
modify docs, add test cases
yeonjuan Feb 7, 2020
ed2485d
add test cases
yeonjuan Feb 7, 2020
c22fe9c
remove wrong test cases
yeonjuan Feb 7, 2020
81ee5d7
change to ignore destructuring on properties never
yeonjuan Feb 10, 2020
54ee384
update documentation
yeonjuan Feb 11, 2020
a644b11
fix typo
yeonjuan Feb 11, 2020
81dc86c
fix documentation
yeonjuan Feb 11, 2020
486b642
include error loc, data info
yeonjuan Feb 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 13 additions & 11 deletions docs/rules/id-length.md
View file
Open in desktop
Expand Up @@ -31,9 +31,9 @@ var myObj = { a: 1 };
class x { }
class Foo { x() {} }
function foo(...x) { }
function foo({x}) { }
var { x } = {};
var { x: a} = {};
var { a: [x]} = {};
var { prop: a} = {};
({ prop: obj.x } = {});
```

Expand All @@ -59,8 +59,10 @@ function foo(num = 0) { }
class MyClass { }
class Foo { method() {} }
function foo(...args) { }
function foo({ prop }) { }
function foo({ a: prop }) { }
var { prop } = {};
var { prop: a } = {};
var { a: prop } = {};
var { prop: [x] } = {};
({ prop: obj.longName } = {});
var data = { "x": 1 }; // excused because of quotes
Expand Down Expand Up @@ -89,8 +91,7 @@ class x { }
class Foo { x() {} }
function foo(...x) { }
var { x } = {};
var { x: a} = {};
var { a: [x]} = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a regression in this PR? I see that this is going to be worked on in a separate PR. Do we have an issue outlining our plan for this (just in case someone else needs to pick this work up)?

Copy link
Member Author

@yeonjuan yeonjuan Feb 16, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaicataldo

Is this a regression in this PR?

Yes. The case will not be warned after this PR merged because the actual version warns about a rather than x.

/*eslint id-length: ["error", { "min": 4 }]*/
/*eslint-env es6*/

var { a: [x]} = {}; // Error: Identifier name `a` ...

And there is a PR working on to fix it (#12839).

Do we have an issue outlining our plan for this (just in case someone else needs to pick this work up)?

I will add an outlining plan for this on #12832. Thanks 👍

var { prop: x} = {};
({ prop: obj.x } = {});
```

Expand All @@ -103,7 +104,7 @@ Examples of **correct** code for this rule with a minimum of 4:
var value = 5;
function func() { return 42; }
obj.element = document.body;
var foo = function (event) { /* do stuff */ };
var foobar = function (event) { /* do stuff */ };
try {
dangerousStuff();
} catch (error) {
Expand All @@ -116,7 +117,7 @@ class MyClass { }
class Foobar { method() {} }
function foobar(...args) { }
var { prop } = {};
var { prop: a } = {};
var { a: longName } = {};
var { prop: [x] } = {};
({ prop: obj.name } = {});
var data = { "x": 1 }; // excused because of quotes
Expand Down Expand Up @@ -153,8 +154,7 @@ class x { }
class Foo { x() {} }
function foo(...x) { }
var { x } = {};
var { x: a} = {};
var { a: [x]} = {};
var { prop: a} = {};
({ prop: obj.x } = {});
```

Expand All @@ -167,7 +167,7 @@ Examples of **correct** code for this rule with the `{ "min": 4 }` option:
var value = 5;
function func() { return 42; }
obj.element = document.body;
var foo = function (event) { /* do stuff */ };
var foobar = function (event) { /* do stuff */ };
try {
dangerousStuff();
} catch (error) {
Expand All @@ -180,7 +180,7 @@ class MyClass { }
class Foobar { method() {} }
function foobar(...args) { }
var { prop } = {};
var { prop: a } = {};
var { a: longName } = {};
var { prop: [x] } = {};
({ prop: obj.name } = {});
var data = { "x": 1 }; // excused because of quotes
Expand Down Expand Up @@ -256,6 +256,8 @@ try {
// ignore as many do
}
(x) => { return x * x; };
const { x } = foo;
const { a: x } = foo;
```

## Related Rules
Expand Down
15 changes: 12 additions & 3 deletions lib/rules/id-length.js
View file
Open in desktop
Expand Up @@ -63,6 +63,7 @@ module.exports = {

return obj;
}, {});
const reportedNode = new Set();

const SUPPORTED_EXPRESSIONS = {
MemberExpression: properties && function(parent) {
Expand All @@ -82,8 +83,15 @@ module.exports = {
VariableDeclarator(parent, node) {
return parent.id === node;
},
Property: properties && function(parent, node) {
return parent.key === node;
Property(parent, node) {

if (parent.parent.type === "ObjectPattern") {
return (
parent.value !== parent.key && parent.value === node ||
parent.value === parent.key && parent.key === node && properties
);
}
return properties && !parent.computed && parent.key === node;
},
ImportDefaultSpecifier: true,
RestElement: true,
Expand All @@ -109,7 +117,8 @@ module.exports = {

const isValidExpression = SUPPORTED_EXPRESSIONS[parent.type];

if (isValidExpression && (isValidExpression === true || isValidExpression(parent, node))) {
if (isValidExpression && !reportedNode.has(node) && (isValidExpression === true || isValidExpression(parent, node))) {
reportedNode.add(node);
context.report({
node,
messageId: isShort ? "tooShort" : "tooLong",
Expand Down