-
-
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
fix: don't remove comments between key and value in object-shorthand #18442
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
lib/rules/object-shorthand.js
Outdated
const lastKeyToken = sourceCode.getLastToken(node.key); | ||
|
||
// x: /* */ x | ||
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) { | ||
return null; | ||
} | ||
|
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.
how about if we give a fix here through appending the comment before or after the property something like:
var x = {
a /* comment */: a,
b
}
can be fixed to
var x = {
/* comment */ a,
b
}
or
var x = {
a, /* comment */
b
}
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 in that case it becomes a feature request, not a bug fix. You can create an issue for this idea.
Whatever the decision is, I'd recommend that both identifiers and function expressions were handled identically (which they aren't right now, hence this PR).
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 it's fine not to provide the fix when there's a comment between the two identifiers and let the author fix this manually as it isn't clear where the new position for the comment should be.
lib/rules/object-shorthand.js
Outdated
const lastKeyToken = sourceCode.getLastToken(node.key); | ||
|
||
// x: /* */ x | ||
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) { | ||
return null; | ||
} |
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.
const lastKeyToken = sourceCode.getLastToken(node.key); | |
// x: /* */ x | |
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) { | |
return null; | |
} | |
// x: /* */ x | |
if (sourceCode.commentsExistBetween(node.key, node.value)) { | |
return null; | |
} |
We can use node.key
directly.
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.
lib/rules/object-shorthand.js
Outdated
const lastKeyToken = sourceCode.getLastToken(node.key); | ||
|
||
// "x": /* */ x | ||
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) { | ||
return null; | ||
} |
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.
const lastKeyToken = sourceCode.getLastToken(node.key); | |
// "x": /* */ x | |
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) { | |
return null; | |
} | |
// "x": /* */ x | |
if (sourceCode.commentsExistBetween(node.key, node.value)) { | |
return null; | |
} |
Same here.
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.
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.
LGTM, thanks!
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.
Just noticed an edge case that wasn't covered (see the new test cases).
lib/rules/object-shorthand.js
Outdated
@@ -497,6 +497,12 @@ module.exports = { | |||
node, | |||
messageId: "expectedPropertyShorthand", | |||
fix(fixer) { | |||
|
|||
// x: /* */ x | |||
if (sourceCode.commentsExistBetween(node.key, node.value)) { |
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.
if (sourceCode.commentsExistBetween(node.key, node.value)) { | |
if (sourceCode.getCommentsInside(node).length > 0) { |
Since we're replacing the entire node
with just an identifier name, seems best to check if there are comments anywhere inside the node
.
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.
lib/rules/object-shorthand.js
Outdated
@@ -510,6 +516,12 @@ module.exports = { | |||
node, | |||
messageId: "expectedPropertyShorthand", | |||
fix(fixer) { | |||
|
|||
// "x": /* */ x | |||
if (sourceCode.commentsExistBetween(node.key, node.value)) { |
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.
if (sourceCode.commentsExistBetween(node.key, node.value)) { | |
if (sourceCode.getCommentsInside(node).length > 0) { |
Same here.
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.
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.
LGTM, thanks!
Fixes #18441
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Check for comments between the last key token and the value in the property node.
Is there anything you'd like reviewers to focus on?