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: Avoid fixing objects containing comments (fixes #8484) #8944
Conversation
LGTM |
@schempy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @not-an-aardvark and @kaicataldo to be potential reviewers. |
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 for 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.
Thanks for the PR. I have a few nitpicks, but they shouldn't block this from getting merged.
"};" | ||
].join("\n"), | ||
output: [ | ||
"var a = {", |
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.
Nitpick: Can you use output: null
for these test cases to indicate that no output is produced, rather than making output
the same as code
?
lib/rules/object-curly-newline.js
Outdated
@@ -144,6 +144,9 @@ module.exports = { | |||
) | |||
); | |||
|
|||
const allTokens = sourceCode.getTokens(node, { includeComments: true }); | |||
const hasComments = allTokens.some(token => astUtils.isCommentToken(token)); |
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.
Nitpick: I think these lines could be replaced with
const hasComments = !!sourceCode.getFirstToken(node, astUtils.isCommentToken);
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.
That change did not work. I'll be happy to make other changes if you'd like. Also, I updated the test files as suggested.
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.
Oops, sorry about that. Does this work?
const hasComments = sourceCode.commentsExistBetween(
sourceCode.getFirstToken(node),
sourceCode.getLastToken(node)
);
The reason I bring this up is because I'm a bit concerned about the performance impact of iterating through every token of every object literal in a file. (In most cases it should probably be fine, but one can imagine a worst-case file that has many very large object literals.) The hasComments
variable is currently declared outside all of the if
statements here, so the comments will be enumerated in all files, not just files where the rule would report an error.
sourceCode.commentsExistBetween
uses binary search to find comments (since it has access to all the comment locations beforehand).
LGTM |
LGTM |
LGTM |
By the way, is it intentional that no fix is performed even when the comment is between properties? For example, no fix will be performed for this object: /* eslint object-curly-newline: [error, always] */
({ foo: 1,
bar: 2,
// comment
baz: 3
}); But it seems like such a fix would clearly be safe. Maybe it would be better to explicitly check between the |
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.
Temporarily requesting changes to make sure no one merges this before we figure out #8944 (comment).
LGTM |
LGTM |
@not-an-aardvark I made updates to check between the |
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.
Thanks! I left a few suggestions.
lib/rules/object-curly-newline.js
Outdated
@@ -172,6 +177,10 @@ module.exports = { | |||
node, | |||
loc: closeBrace.loc.start, | |||
fix(fixer) { | |||
if (hasComments) { |
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.
For this check, I think it would matter whether the last token of the object is a comment, not the first.
lib/rules/object-curly-newline.js
Outdated
@@ -206,6 +219,10 @@ module.exports = { | |||
node, | |||
loc: closeBrace.loc.start, | |||
fix(fixer) { | |||
if (hasComments) { |
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.
For this check, I think it would matter whether the last token of the object is a comment, not the first.
Sure, feel free. It might be good to mention |
LGTM |
References rule object-curly-newline. Avoid fixing line breaks when the token after opening brace is a comment or token before closing brace is a comment.
LGTM |
Thanks for the feedback! Code updates and commit message have been made. |
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.
Looks good to me. Thanks for contributing!
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)
Avoid fixing objects containing comments proposed by @not-an-aardvark in PR #8487