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

Update: add comment to message in no-warning-comments (fixes #12327) #13522

Merged
merged 8 commits into from Aug 29, 2020

Conversation

anikethsaha
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] 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)

Added data in the error message which will show the comment itself.
It will only show till 30 characters

image

Is there anything you'd like reviewers to focus on?

Is 30 is fine ? after 30 char it will be truncated.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 22, 2020
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jul 22, 2020
@mdjermanovic
Copy link
Member

Should we avoid linebreaks in multiline comments, for example for this one in linter.js the rule would output:

542:5  error  Unexpected 'todo' comment: * TODO: @aladdin-add
     * 1...  no-warning-comments

Also, if it isn't too difficult, it might be nice to avoid cutting words, surrogate pairs, and composite characters?

lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

Should we avoid linebreaks in multiline comments, for example for this one in linter.js the rule would output:

542:5  error  Unexpected 'todo' comment: * TODO: @aladdin-add
     * 1...  no-warning-comments

Or can we add \n when there is a linebreak ?

Also, if it isn't too difficult, it might be nice to avoid cutting words, surrogate pairs, and composite characters?

I didn't get this

@mdjermanovic
Copy link
Member

Should we avoid linebreaks in multiline comments, for example for this one in linter.js the rule would output:

542:5  error  Unexpected 'todo' comment: * TODO: @aladdin-add
     * 1...  no-warning-comments

Or can we add \n when there is a linebreak ?

Sorry, I didn't understand this, how would the output look like? I think we're usually trying to avoid linebreaks in final messages. In this case, the final message has a linebreak taken from the source code.

Also, if it isn't too difficult, it might be nice to avoid cutting words, surrogate pairs, and composite characters?

I didn't get this

For example, for this comment, should the message be:

Unexpected 'todo' comment: * If the term ends in a word c...

or

Unexpected 'todo' comment: * If the term ends in a word...

Also, slice might accidentally cut some code units that should be together:

"👍".slice(0, 1) // �

@anikethsaha
Copy link
Member Author

Sorry, I didn't understand this, how would the output look like? I think we're usually trying to avoid linebreaks in final messages. In this case, the final message has a linebreak taken from the source code.

like this I meant

comment.replace('\n', '\\n')

So it will be like

542:5  error  Unexpected 'todo' comment: * TODO: @aladdin-add \n  * 1...  no-warning-comments

WDYT ?

For example, for this comment, should the message be:

Unexpected 'todo' comment: * If the term ends in a word c...

or

Unexpected 'todo' comment: * If the term ends in a word...

Also, slice might accidentally cut some code units that should be together:

"👍".slice(0, 1) // �

OKay, I will try to implement the same without slice

@mdjermanovic
Copy link
Member

Maybe we could split the comment by whitespace, and then concatenate the parts with 1 space as a separator while the total length doesn't exceed the max?

I guess that would solve all problems with linebreaks, breaking words, surrogate pairs, etc. The comment wouldn't look exactly like in the source code, but the significant content would be shown.

@anikethsaha
Copy link
Member Author

Maybe we could split the comment by whitespace, and then concatenate the parts with 1 space as a separator while the total length doesn't exceed the max?

I guess that would solve all problems with linebreaks, breaking words, surrogate pairs, etc. The comment wouldn't look exactly like in the source code, but the significant content would be shown.

yes, I have the similar thought.
Though I was thinking of adding \n as mentioned above comment in the linebreak so that the user can know that there is a linebreak instead of simply copying the text and searching for a particular comment in the file.

WDYT ?

@mdjermanovic
Copy link
Member

Though I was thinking of adding \n as mentioned above comment in the linebreak so that the user can know that there is a linebreak instead of simply copying the text and searching for a particular comment in the file.

I don't have a strong opinion on this. It might be useful for someone (though we also report comment's line), but it might be seen as extra characters that negatively affect readability by someone else.

lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic changed the title Update: added the comment in no-warning-comments (fixes #12327) Update: add comment to message in no-warning-comments (fixes #12327) Aug 12, 2020
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
lib/rules/no-warning-comments.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@kaicataldo kaicataldo merged commit ed64767 into eslint:master Aug 29, 2020
@kaicataldo
Copy link
Member

Thanks for contributing!

@fregante
Copy link
Contributor

Awesome!

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 18, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 18, 2021
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-warning-comments should include the comment itself in the report (UX only)
4 participants