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.

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

Merged
merged 1 commit into from Jul 21, 2017

Conversation

schempy
Copy link
Contributor

@schempy schempy commented Jul 14, 2017

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

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@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.

Copy link
Member

@ilyavolodin ilyavolodin left a 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

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules labels Jul 16, 2017
Copy link
Member

@not-an-aardvark not-an-aardvark left a 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 = {",
Copy link
Member

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?

@@ -144,6 +144,9 @@ module.exports = {
)
);

const allTokens = sourceCode.getTokens(node, { includeComments: true });
const hasComments = allTokens.some(token => astUtils.isCommentToken(token));
Copy link
Member

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);

Copy link
Contributor Author

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.

Copy link
Member

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).

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member

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 { and the first token in the object, rather than checking for comments anywhere in the object.

Copy link
Member

@not-an-aardvark not-an-aardvark left a 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).

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@schempy
Copy link
Contributor Author

schempy commented Jul 19, 2017

@not-an-aardvark I made updates to check between the { and the first token in the object for comments. Updated tests as well. Do you think I should update the commit message since this is not avoiding fixing objects containing comments?

Copy link
Member

@not-an-aardvark not-an-aardvark left a 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.

@@ -172,6 +177,10 @@ module.exports = {
node,
loc: closeBrace.loc.start,
fix(fixer) {
if (hasComments) {
Copy link
Member

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.

@@ -206,6 +219,10 @@ module.exports = {
node,
loc: closeBrace.loc.start,
fix(fixer) {
if (hasComments) {
Copy link
Member

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.

@not-an-aardvark
Copy link
Member

Do you think I should update the commit message since this is not avoiding fixing objects containing comments?

Sure, feel free. It might be good to mention object-curly-spacing in the commit message as well.

@eslintbot
Copy link

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.
@eslintbot
Copy link

LGTM

@schempy
Copy link
Contributor Author

schempy commented Jul 20, 2017

Thanks for the feedback! Code updates and commit message have been made.

Copy link
Member

@not-an-aardvark not-an-aardvark left a 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!

@ilyavolodin ilyavolodin merged commit 96df8c9 into eslint:master Jul 21, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants