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
Bug: no-warning-comments does not work with multiline comments #16103
Comments
Hi @Reeywhaar, thanks for the issue! We were just discussing this in #16090, and it doesn't seem that the original issue #9884 was actually resolved at the time.
I agree that this rule should treat these two comments as starting with 'todo' despite the extra |
I actually thought about this issue as I was working on #16090. I thought the original intent of location "start" was only to match the start of a single line comment, so I kept it that way in my changes. Setting location to "anywhere" does work more usefully for block comments. Fixing this, however, would require adding the multiline ( If backwards compatibility is a concern, a possible way to do this would be to introduce a new location value that handled this use case better for block comments, particularly if people consider that location "anywhere" doesn't meet their needs well enough. |
I am not expert in |
I had a go at stripping decorative punctuation in #16120 based on that previous discussion. The difficulty with this, though, is that there needs to be a clearly agreed upon definition of what counts as decorative punctuation in comments that should be stripped. Rather than just stripping all leading and trailing punctuation, I looked for sequences of a few specific characters, and I picked characters that seemed like the most common to be encountered. Specifically, It's possible there are edge cases that I missed, and it's also possible there are other edge cases where it shouldn't strip punctuation, but it does. The documentation for no-warning-comments should also get updated to explain the new functionality before merging it. |
I think this issue is only about comments that have "decoration" characters before the effective text starts. If we add the /*
this is not a
todo comment
*/ |
A problem with removing some characters from the comment and then searching for terms in the stripped text is that terms in existing configurations can contain those characters. For example, if we remove all leading /* eslint no-warning-comments: ["error", { terms: ["*TODO*"] } ] */
/*
*TODO* add something
*/ A backward-compatible solution could be to keep the current behavior as is, and add an option to specify characters that should be skipped at the start of comments. |
That was indeed my interpretation of the issue: Matching the start of any line in a comment, rather than just the start of the first line. But implementing it the way you suggest would just require stripping decorative punctuation characters and leading whitespace up to the first non-punctuation character. It could be simplified further by only stripping asterisks and whitespace, if that's the desired behaviour. There could still be edge cases, like for example: { terms: ["*TODO*"] }
/*
*TODO* something
*/ Maybe instead of hardcoding the characters considered decorative, that could be configurable, with a sensible default setting.
|
I believe the current behavior is intentional. I wouldn’t mind adding an option to support the JSDoc-style behavior, maybe allowing a user to specify the text to be skipped at the start of the comment? I don’t think there’s a good heuristic for knowing which characters after the first asterisk should and shouldn’t be considered by this rule automatically. |
As a workaround I've tried to amend rule as follows:
And failed :-) Also, I think I should mark that comment can be indented:
So |
@eslint/eslint-tsc what do we think about adding a new option for the desired behavior? |
I agree that this should be fixed by adding a new option. The option could accept an array of characters to (optionally) skip at the start of the comment when checking whether it starts with disallowed terms? Similar to #16103 (comment): "no-warning-comments": [1, {
terms: ["TODO"],
location: "start",
commentDecorationChars: ["*"]
}] These characters would be added to the |
I like it. 👍 |
@mdjermanovic's suggestion looks good! |
I updated my PR to address the comments here. But I went with a single given { decoration: "*" } The regex becomes |
String as a list of characters might be confusing. For example, |
I thought about, but there's no easy way to enforce that each string in the array is a single character anyway. I could change it to allow either a string or array or strings, and then explain in the docs that they're equivalent. |
We can use commentDecorationChars: {
type: "array",
items: {
type: "string",
pattern: "^\\S$"
},
minItems: 1,
uniqueItems: true
}
|
There is exact same issue, that marked as resolved, but it seems there is regression?
Previous issue: #9884
I will copy description from linked issue, except I've updated the environment.
Environment
Node version: 18.2.0
npm version: 8.9.0
Local ESLint version: 8.13.0
Global ESLint version: 8.13.0
Operating System: macOS 12.4
What parser are you using?
@babel/eslint-parser
What did you do?
Configuration
Run it in the online demo.
What did you expect to happen?
The sections
and
should be flagged for
no-warning-comments
.What actually happened?
Only
is flagged for
no-warning-comments
.Participation
The text was updated successfully, but these errors were encountered: