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

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 An ESLint team member will look at this issue soon label Mar 11, 2020
@kaicataldo
Copy link
Member

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 This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 20, 2020
@mdjermanovic
Copy link
Member

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

@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

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

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

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

@kaicataldo
Copy link
Member

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 pull request May 15, 2020
nzakas added a commit to eslint/archive-website that referenced this pull request May 15, 2020
@kaicataldo
Copy link
Member

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

@nzakas nzakas 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 19, 2020
@nzakas
Copy link
Member

nzakas commented Jun 19, 2020

I've added my 👍 so marking as accepted.

@mdjermanovic
Copy link
Member

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

@mdjermanovic
Copy link
Member

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

@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 August 11, 2020 12:18
@EdieLemoine
Copy link
Contributor Author

@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

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.

lib/rules/no-inline-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.

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
Copy link
Contributor Author

@mdjermanovic I updated the branch.

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.

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
Copy link
Contributor Author

@mdjermanovic I updated it again :)

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.

LGTM, thanks!

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.

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

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

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

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

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

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 September 26, 2020 20:55

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
mdjermanovic pushed a commit that referenced this pull request 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 September 27, 2020 12:05
@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 This issue has been archived; please open a new issue for any further discussion label Mar 26, 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-inline-comments exception for istanbul ignore comments
5 participants