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.

Sign up for GitHub

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

Jump to bottom

Update: Fixes multiline no-warning-comments rule. (fixes #9884) #10381

Merged
merged 1 commit into from
Jun 9, 2018
Merged

Update: Fixes multiline no-warning-comments rule. (fixes #9884) #10381

merged 1 commit into from
Jun 9, 2018

Conversation

sstern6
Copy link
Contributor

@sstern6 sstern6 commented May 20, 2018 •
edited

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)
Updated the RegExp to look for the term provided in the body of the comment, not just the beginning.

Is there anything you'd like reviewers to focus on?
Please review new test cases add/moved from valid to invalid. The examples on the documentation were not throwing errors https://eslint.org/docs/2.0.0/rules/no-warning-comments as stated, ie:

Previous to the PR was not throwing any errors.

/*
*Unexpected 'any other term' comment.
 * The same goes for this TODO comment
 * Or a fixme
 * as well as any other term
*/

Only had 1 error not 2 errors:

// todo and fixme

Thank you

Fixes: #9884

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 20, 2018
platinumazure
platinumazure requested changes May 20, 2018
View reviewed changes
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.

Hi @sstern6, thanks for contributing!

Apologies, I've only now had a chance to look at the rule docs fully and I think I led you astray.

There's a location option in the rule, which can be "start" or "anywhere" (defaults to "start"). I think we only want to make changes for { location: "anywhere" } at this point. (For { location: "start" }, the only change we would want to make is to allow some extra non-word characters at the start of the comment before identifying the real "starting word" of the comment, but that can/should be done in a separate PR.)

It might be worth stepping through the code with { location: "anywhere" } to see how that code works and what alternate paths it takes. Does it generate a different regex? etc.

I've left comments on the test cases that I think need to be changed. Let me know if anything doesn't make sense. Sorry for the confusion I accidentally led you into. Thanks!

lib/rules/no-warning-comments.js Outdated
} else {
prefix = "";
}

return new RegExp(prefix + escaped + suffix, "i");
return new RegExp(prefix + escaped + suffix + eitherOrWordBoundary + term + wordBoundary, "i");
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 really helpful to have a comment here showing what the regex should look like for an example term.

Also: Based on the issues I've identified in the tests, we want to make sure that this regex is only augmented when { location: "anywhere" } option is specified.

tests/lib/rules/no-warning-comments.js Outdated
@@ -24,15 +24,10 @@ ruleTester.run("no-warning-comments", rule, {
{ code: "// any comment", options: [{ terms: ["fixme", "todo"] }] },
"// any comment",
{ code: "// any comment", options: [{ location: "anywhere" }] },
{ code: "// any comment with TODO, FIXME or XXX", options: [{ location: "start" }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be valid: the rule option { location: "start" } means only flag the comment if it starts with TODO/FIXME/XXX.

tests/lib/rules/no-warning-comments.js Outdated
@@ -24,15 +24,10 @@ ruleTester.run("no-warning-comments", rule, {
{ code: "// any comment", options: [{ terms: ["fixme", "todo"] }] },
"// any comment",
{ code: "// any comment", options: [{ location: "anywhere" }] },
{ code: "// any comment with TODO, FIXME or XXX", options: [{ location: "start" }] },
"// any comment with TODO, FIXME or XXX",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be valid as well: { location: "start" } appears to be the rule default. This should only be flagged if the { location: "anywhere" } option is explicitly specified.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* any block comment */", options: [{ terms: ["fixme"] }] },
{ code: "/* any block comment */", options: [{ terms: ["fixme", "todo"] }] },
"/* any block comment */",
{ code: "/* any block comment */", options: [{ location: "anywhere" }] },
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be valid since the term is not at the start of the comment.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* any block comment */", options: [{ terms: ["fixme"] }] },
{ code: "/* any block comment */", options: [{ terms: ["fixme", "todo"] }] },
"/* any block comment */",
{ code: "/* any block comment */", options: [{ location: "anywhere" }] },
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }] },
"/* any block comment with TODO, FIXME or XXX */",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be valid since the term is not at the start of the comment.

tests/lib/rules/no-warning-comments.js Outdated
@@ -50,11 +45,15 @@ ruleTester.run("no-warning-comments", rule, {
{ code: "// any fixme or todo", options: [{ terms: ["fixme", "todo"], location: "anywhere" }], errors: [{ message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'todo' comment." }] },
{ code: "/* any fixme or todo */", options: [{ terms: ["fixme", "todo"], location: "anywhere" }], errors: [{ message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'todo' comment." }] },
{ code: "/* any fixme or todo */", options: [{ location: "anywhere" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }] },
{ code: "/* fixme and todo */", errors: [{ message: "Unexpected 'fixme' comment." }] },
{ code: "/* fixme and todo */", errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be invalid with 2 messages if { location: "anywhere" } is specified as an option.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* any fixme */", options: [{ location: "anywhere" }], errors: [{ message: "Unexpected 'fixme' comment." }] },
{ code: "/* fixme! */", options: [{ terms: ["fixme"] }], errors: [{ message: "Unexpected 'fixme' comment." }] },
{ code: "// regex [litera|$]", options: [{ terms: ["[litera|$]"], location: "anywhere" }], errors: [{ message: "Unexpected '[litera|$]' comment." }] },
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["eslint"] }], errors: [{ message: "Unexpected 'eslint' comment." }] },
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["one"], location: "anywhere" }], errors: [{ message: "Unexpected 'one' comment." }] }
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["one"], location: "anywhere" }], errors: [{ message: "Unexpected 'one' comment." }] },
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be invalid when { location: "anywhere" } is specified.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["one"], location: "anywhere" }], errors: [{ message: "Unexpected 'one' comment." }] }
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["one"], location: "anywhere" }], errors: [{ message: "Unexpected 'one' comment." }] },
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "/* any block comment with (TODO, FIXME's or XXX!) */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be invalid when { location: "anywhere" } is specified.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* eslint one-var: 2 */", options: [{ terms: ["one"], location: "anywhere" }], errors: [{ message: "Unexpected 'one' comment." }] },
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "/* any block comment with (TODO, FIXME's or XXX!) */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "/** \n *any block comment \n*with (TODO, FIXME's or XXX!) **/", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be invalid when { location: "anywhere" } is specified.

tests/lib/rules/no-warning-comments.js Outdated
{ code: "/* any block comment with TODO, FIXME or XXX */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "/* any block comment with (TODO, FIXME's or XXX!) */", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "/** \n *any block comment \n*with (TODO, FIXME's or XXX!) **/", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] },
{ code: "// any comment with TODO, FIXME or XXX", options: [{ location: "start" }], errors: [{ message: "Unexpected 'todo' comment." }, { message: "Unexpected 'fixme' comment." }, { message: "Unexpected 'xxx' comment." }] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be invalid when { location: "anywhere" } is specified.

@platinumazure
Copy link
Member

Oh, forgot to mention: This should be an "Update" since this is a bugfix that potentially increases the number of warnings the user might experience. Please change the commit message accordingly. Thanks!

@platinumazure platinumazure added bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 20, 2018
@sstern6
Copy link
Contributor Author

sstern6 commented May 20, 2018 •
edited

Thanks for the feedback @platinumazure addressed all comments and split the logic between location start and location anywhere for the appropriate RegExp with some comments. Let me know if there are any more comments to address

Thanks

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 7, 2018
@kaicataldo
Copy link
Member

Sorry for losing track of this. Marking this as accepted, since the corresponding issue is accepted. @sstern6 Do you mind updating the description to reference the issue?

@platinumazure have your comments been addressed?

@platinumazure
Copy link
Member

Apologies @sstern6 and @kaicataldo, yes, my concerns are addressed.

kaicataldo
kaicataldo approved these changes Jun 7, 2018
View reviewed changes
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.

LGTM. Thanks for contributing to ESLint!

platinumazure
platinumazure previously requested changes Jun 9, 2018
View reviewed changes
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.

I apologize, I missed one thing.

Since this fix will increase warnings, this needs to be prefixed with "Update:" instead of "Fix:". Once that is done, this is ready to go. Thanks and sorry for missing that!

@kaicataldo kaicataldo dismissed platinumazure’s stale review June 9, 2018 15:51

Renaming this when we squash and merge to ensure this gets into today's RC release

@kaicataldo kaicataldo changed the title Fix: Fixes multiline no-warning-comments rule. (fixes #9884) Update: Fixes multiline no-warning-comments rule. (fixes #9884) Jun 9, 2018
@kaicataldo kaicataldo merged commit 640bf07 into eslint:master Jun 9, 2018
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 8, 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 Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaicataldo kaicataldo kaicataldo approved these changes

@platinumazure platinumazure platinumazure left review comments

Assignees
No one assigned
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no-warning-comments does not work with multiline comments
3 participants
@sstern6 @platinumazure @kaicataldo