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 ignorePattern to no-inline-comments #13029
Update: Add ignorePattern to no-inline-comments #13029
Conversation
8e90b44
to
fc1b5c4
Compare
Thanks for the PR! While I think this is a use-case that would be good to support, I personally don't think this is the correct solution. The current ignored comments are all either ESLint directive comments or comments for other linters (I believe this is to ease the pain of switching to ESLint from I would advocate for adding a new option to Please note that we'll need to go through the consensus process outlined here before we can merge anything. Feel free to wait implementing anything else until we come to a decision so that you don't have to spend more time on it! @eslint/eslint-team Thoughts? |
This option already allows users to define custom patterns like those for webpack magic comments, so I don't think we should change the default pattern ( As for the |
@EdieLemoine Would you be interested in helping us implement the suggestion above once we come to a consensus on its inclusion? |
@kaicataldo sure. Just let me know which solution you guys prefer. |
I will champion this change (as described here). Given the two 👍 on my proposal, we only need one more 👍. |
And thanks @EdieLemoine! Once we reach consensus we'll make sure it's clear here (hopefully soon since we only need one more 👍). |
@eslint/eslint-team We just need one more 👍 to accept this. |
I've added my 👍 so marking as accepted. |
Linked #12755 as it seems that this option will fix the issue with |
to clarify, should the option accept an array of "regex" strings or just one string? The same option in similar rules like |
My take is that we should match what those other rules are currently doing. Supporting RegExp strings would be an additive change, and we can add it to all the rules listed if we decide we want to go that route in the future. |
@EdieLemoine Thanks for your patience! Is this still something you're willing to help out with? |
8ea8844
to
f819bd0
Compare
@kaicataldo I just updated it. Is this the proper solution? |
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.
@EdieLemoine thanks for the changes!
Yes, it is accepted to add ignorePatterns
, just as you implemented.
But, applying astUtils.COMMENTS_IGNORE_PATTERN
wasn't considered, and that would be a breaking change. I left a note on the line that should be removed.
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.
ignorePattern
looks good, I left just a couple of notes about tests and the schema.
The new option should be also documented in no-inline-comments.md
f819bd0
to
36e4a2c
Compare
@mdjermanovic I updated the branch. |
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! Looks great, I have just a few minor suggestions.
36e4a2c
to
3e7bcae
Compare
@mdjermanovic I updated it again :) |
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!
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.
This looks really great - thanks! One small comment about clarifying the docs, but otherwise LGTM.
|
||
### ignorePattern | ||
|
||
A regular expression can be provided to make this rule ignore specific comments. |
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.
Can we clarify that this accepts a string rather than a RegExp literal?
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.
Hmm, it's implemented the same way as in lines-around-comment. It's referenced to as a "regular expression" there as well.
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.
That's a good point, @EdieLemoine. Since that's the only change @kaicataldo requested, let's merge this to get it into today's release, and I'll put together a quick PR to change this in both places.
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 contributing, @EdieLemoine! I'll update the docs after merging, and this will be in today's release.
|
||
### ignorePattern | ||
|
||
A regular expression can be provided to make this rule ignore specific comments. |
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.
That's a good point, @EdieLemoine. Since that's the only change @kaicataldo requested, let's merge this to get it into today's release, and I'll put together a quick PR to change this in both places.
I'll update the docs in both places after merging since this was good to go otherwise.
Update: Add filtering to no-inline-comments and allow Webpack magic comments
Prerequisites checklist
What is the purpose of this pull request?
What rule do you want to change?
no-inline-comments
Does this change cause the rule to produce more or fewer warnings?
Fewer
How will the change be implemented?
New default behaviour.
Please provide some example code that this change will affect:
What does the rule currently do for this code?
Reports error/warning:
Unexpected comment inline with code.(no-inline-comments)
What will the rule do after it's changed?
Not report on this kind of comment anymore.
What changes did you make?
I added all the [Webpack magic comments](https://webpack.js.org/api/module-methods/#magic-comments] keywords to the
astUtils.COMMENTS_IGNORE_PATTERN
regex. It looks like this now:This affects the following rules:
I added a filter to the
sourceCode.getAllComments()
call in theProgram
method of the rule's definition so these comments (and the ones that were already present inCOMMENTS_IGNORE_PATTERN
) will be ignored.Is there anything you'd like reviewers to focus on?
Is the
astUtils.COMMENTS_IGNORE_PATTERN
is the correct place for the new exceptions? If so, I will update the docs in every place it applies.Should this be the new default behaviour?