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: captialized-comments fatal error fixed (fixes #7663) #7664

Merged
merged 1 commit into from Nov 27, 2016

Conversation

Trott
Copy link
Contributor

@Trott Trott commented Nov 26, 2016

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 cause capitalized-comments to terminate eslint with a TypeError.

Added code to make sure that firstWordChar is an empty string rather than undefined in that case, thus avoiding the TypeError.

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.

@mention-bot
Copy link

@Trott, thanks for your PR! By analyzing the history of the files in this pull request, we identified @platinumazure to be a potential reviewer.

@jsf-clabot
Copy link

jsf-clabot commented Nov 26, 2016

CLA assistant check
All committers have signed the CLA.

@eslintbot
Copy link

LGTM

Copy link
Member

@kaicataldo kaicataldo 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 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 */",
"// ",
Copy link
Member

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*/",

Copy link
Contributor Author

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

@eslintbot
Copy link

LGTM

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! 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] || "";
Copy link
Member

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.

Copy link
Contributor Author

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.

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

Cripes, sorry for missing this case.

Copy link
Member

@platinumazure platinumazure left a 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?

@platinumazure
Copy link
Member

My review comment was unclear- I'm requesting test cases for other line terminators, just in case.

@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 patch candidate This issue may necessitate a patch release in the next few days rule Relates to ESLint's core rules labels Nov 27, 2016
@Trott
Copy link
Contributor Author

Trott commented Nov 27, 2016

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

LGTM

@Trott
Copy link
Contributor Author

Trott commented Nov 27, 2016

Added additional tests as requested by reviewers. Anything else?

Copy link
Member

@platinumazure platinumazure 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!

@kaicataldo
Copy link
Member

LGTM. Thanks for contributing!

@kaicataldo kaicataldo merged commit be739d0 into eslint:master Nov 27, 2016
@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 patch candidate This issue may necessitate a patch release in the next few days rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants