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 ignorePattern to no-inline-comments #13029

Merged
merged 1 commit into from Sep 26, 2020

Conversation

EdieLemoine
Copy link
Contributor

@EdieLemoine EdieLemoine commented Mar 11, 2020

Update: Add filtering to no-inline-comments and allow Webpack magic comments

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain:

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:

const language = import(/* webpackChunkName: "my-chunk-name" */'./locale/language');

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:

// From this:
/^\s*(?:eslint|jshint\s+|jslint\s+|istanbul\s+|globals?\s+|exported\s+|jscs)/u
// To this:
/^\s*(?:eslint|jshint\s+|jslint\s+|istanbul\s+|globals?\s+|exported\s+|jscs|webpackInclude:|webpackExclude:|webpackChunkName:|webpackMode:|webpackPrefetch:|webpackPreload:)/u

This affects the following rules:

  • no-inline-comments
  • capitalized-comments (+ This is a big plus as it would break the keywords)
  • line-comment-position (- Likely not necessary)
  • lines-around-comment (- Likely not necessary)
  • multiline-comment-style (- Likely not necessary)

I added a filter to the sourceCode.getAllComments() call in the Program method of the rule's definition so these comments (and the ones that were already present in COMMENTS_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?

@eslint-deprecated eslint-deprecated bot added the triage label Mar 11, 2020
@EdieLemoine EdieLemoine force-pushed the allow-webpack-magic-comments branch from 8e90b44 to fc1b5c4 Compare Mar 11, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Mar 20, 2020

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 jshint, jscs, etc.).

I would advocate for adding a new option to no-inline-comments that allows you to specify an array of exceptions. Maybe ignorePattern to match the option in capitalized-comments?

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?

@kaicataldo kaicataldo added enhancement evaluating rule and removed triage labels Mar 20, 2020
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Mar 20, 2020

line-comment-position, capitalized-comments and lines-around-comment rules have ignorePattern option.

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 (astUtils.COMMENTS_IGNORE_PATTERN) which affects all these rules. It looks like an unnecessary breaking change.

As for the no-inline-comments rule, I believe it should have the same ignorePattern option (maybe applyDefaultIgnorePatterns as well). There is already an open issue #12755 for this enhancement.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Mar 22, 2020

@EdieLemoine Would you be interested in helping us implement the suggestion above once we come to a consensus on its inclusion?

@EdieLemoine
Copy link
Contributor Author

@EdieLemoine EdieLemoine commented Mar 24, 2020

@kaicataldo sure. Just let me know which solution you guys prefer.

@kaicataldo kaicataldo self-assigned this Mar 25, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Mar 25, 2020

I will champion this change (as described here). Given the two 👍 on my proposal, we only need one more 👍.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Mar 25, 2020

And thanks @EdieLemoine! Once we reach consensus we'll make sure it's clear here (hopefully soon since we only need one more 👍).

nzakas added a commit to eslint/archive-website that referenced this issue May 15, 2020
nzakas added a commit to eslint/archive-website that referenced this issue May 15, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jun 13, 2020

@eslint/eslint-team We just need one more 👍 to accept this.

@nzakas nzakas added accepted and removed evaluating labels Jun 19, 2020
@nzakas
Copy link
Member

@nzakas nzakas commented Jun 19, 2020

I've added my 👍 so marking as accepted.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 20, 2020

Linked #12755 as it seems that this option will fix the issue with istanbul comments, too.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 20, 2020

@kaicataldo

I would advocate for adding a new option to no-inline-comments that allows you to specify an array of exceptions. Maybe ignorePattern to match the option in capitalized-comments?

to clarify, should the option accept an array of "regex" strings or just one string?

The same option in similar rules like line-comment-position, capitalized-comments and lines-around-comment accepts 1 string, which provides basically the same functionality (maybe just a bit less convenient in some cases).

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jun 25, 2020

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.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 31, 2020

@EdieLemoine Thanks for your patience! Is this still something you're willing to help out with?

@EdieLemoine EdieLemoine force-pushed the allow-webpack-magic-comments branch 2 times, most recently from 8ea8844 to f819bd0 Compare Aug 11, 2020
@EdieLemoine
Copy link
Contributor Author

@EdieLemoine EdieLemoine commented Aug 11, 2020

@kaicataldo I just updated it. Is this the proper solution?

@mdjermanovic mdjermanovic changed the title Update: Add filtering to no-inline-comments and allow Webpack magic comments Update: Add ignorePattern to no-inline-comments Aug 12, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

lib/rules/no-inline-comments.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

lib/rules/no-inline-comments.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inline-comments.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inline-comments.js Show resolved Hide resolved
@EdieLemoine EdieLemoine force-pushed the allow-webpack-magic-comments branch from f819bd0 to 36e4a2c Compare Aug 12, 2020
@EdieLemoine
Copy link
Contributor Author

@EdieLemoine EdieLemoine commented Aug 12, 2020

@mdjermanovic I updated the branch.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Thanks! Looks great, I have just a few minor suggestions.

docs/rules/no-inline-comments.md Outdated Show resolved Hide resolved
lib/rules/no-inline-comments.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inline-comments.js Show resolved Hide resolved
docs/rules/no-inline-comments.md Outdated Show resolved Hide resolved
@EdieLemoine EdieLemoine force-pushed the allow-webpack-magic-comments branch from 36e4a2c to 3e7bcae Compare Aug 12, 2020
@EdieLemoine
Copy link
Contributor Author

@EdieLemoine EdieLemoine commented Aug 12, 2020

@mdjermanovic I updated it again :)

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

kaicataldo
kaicataldo previously requested changes Aug 13, 2020
Copy link
Member

@kaicataldo kaicataldo left a comment

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.
Copy link
Member

@kaicataldo kaicataldo Aug 13, 2020

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?

Copy link
Contributor Author

@EdieLemoine EdieLemoine Aug 13, 2020

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.

Copy link
Member

@btmills btmills Sep 26, 2020

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.

Copy link
Member

@btmills btmills left a comment

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.
Copy link
Member

@btmills btmills Sep 26, 2020

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.

@btmills btmills dismissed kaicataldo’s stale review Sep 26, 2020

I'll update the docs in both places after merging since this was good to go otherwise.

@btmills btmills merged commit 07d9bea into eslint:master Sep 26, 2020
12 checks passed
mdjermanovic pushed a commit that referenced this issue Sep 26, 2020
…3718)

* Docs: Clarify that ignorePattern should be a string (refs #13029)

* Clarify ignorePattern option adds to default patterns
@EdieLemoine EdieLemoine deleted the allow-webpack-magic-comments branch Sep 27, 2020
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 26, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age label Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age enhancement rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants