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
Update: fix counting jsx comment len in max-len (fixes #12213) #12661
Conversation
|
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 working on this! Do you mind adding some tests for cases where a JSXExpressionContainer
node might contain an expression in addition to a comment?
const example = <A
attr={a && b /* comment */}
></A>
const example2 = <A
attr={/* comment */ a && b}
></A>
@kaicataldo sure! |
@kaicataldo Friendly ping :) |
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 very good, thanks! I left two questions about some edge cases.
I think the decision to treat the whole { /* */ }
as a comment only if everything incl. braces is on the same line is a good decision, since it's a minimal change in behavior.
@kaicataldo @mdjermanovic |
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! Just some minor details
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! Just some detail that could be missing in otherwise very comprehensive tests.
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!
Just to note that this change can produce new warnings if someone has comments max len < code max len configured, which is probably quite rare but maybe this should be labeled as a breaking change?
/* eslint max-len: ["error", {
code: 100,
comments: 50
}]*/
var jsx = (
<p>
{/* this line will be a new error, because it becomes a comment line with > 50 characters */}
</p>
)
I think it's fine to accept this as a semver-minor bug fix (though, in practice I'm not sure it matters since we are merging breaking changes in preparation for v8 right now). |
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, thank you!
Thanks for contributing to ESLint! |
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)
This pr will fix #12213.
If the comment is in the single line jsx comment, change the comment node for checking with JSXExpressionContainer.
ex)
Is there anything you'd like reviewers to focus on?