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: captialized-comments fatal error fixed (fixes #7663) #7664
Conversation
@Trott, thanks for your PR! By analyzing the history of the files in this pull request, we identified @platinumazure to be a potential reviewer. |
LGTM |
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 report and for fixing this! Looks good, but would love a few more tests added.
@@ -40,6 +40,7 @@ ruleTester.run("capitalized-comments", rule, { | |||
"//\xDCber", | |||
"//\u03A0", | |||
"/* Uppercase\nsecond line need not be uppercase */", | |||
"// ", |
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.
Mind adding some more test cases? My thoughts:
// No options: Skips comments that only contain whitespace
"// ",
"//\t",
"/* */",
"/*\t*/",
"/*\n*/",
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.
Sounds good to me. Added. Thanks. (I have the branch set up so that ESLint Members can push directly to it, so if other desirable whitespace tests come to mind, feel free to add them yourself if that's easier/faster.)
LGTM |
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! This LGTM, but I have a suggestion to improve the fix.
@@ -232,7 +232,7 @@ module.exports = { | |||
const commentWordCharsOnly = commentWithoutAsterisks | |||
.replace(WHITESPACE, ""); | |||
|
|||
const firstWordChar = commentWordCharsOnly[0]; | |||
const firstWordChar = commentWordCharsOnly[0] || ""; |
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 would be better to put an explicit length
check above this line, rather than defaulting to an empty string. In all other cases firstWordChar
will be exactly one character long. Setting it to an empty string in this case might cause confusion/unexpected behavior if the code below this line is refactored and someone forgets to account for the empty-string case.
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.
Definitely an improvement. Done. Thanks.
LGTM |
Cripes, sorry for missing this case. |
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.
Maybe we should have the other line terminators (\r
, \r\n
, \u2028
, \u2029
) just in case?
My review comment was unclear- I'm requesting test cases for other line terminators, just in case. |
@platinumazure Do you mean add these test cases? "/*\r*/",
"/*\r\n*/",
"/*\u2028*/",
"/*\u2029*/", |
Fix issue where comments containing a single space would cause eslint to terminate with a TypeError.
LGTM |
Added additional tests as requested by reviewers. Anything else? |
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!
LGTM. 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:
#7663
What changes did you make? (Give an overview)
Added a test to confirm that
//
would causecapitalized-comments
to terminateeslint
with aTypeError
.Added code to make sure that
firstWordChar
is an empty string rather thanundefined
in that case, thus avoiding theTypeError
.Is there anything you'd like reviewers to focus on?
Nope, nothing in particular.
Fix issue where comments containing a single space would cause eslint to
terminate with a TypeError.