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

Chore: Adopt eslint-plugin/require-meta-docs-description internally #12762

Merged

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Jan 9, 2020

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:

What changes did you make? (Give an overview)

This PR replaces the internal-rules/consistent-docs-description lint rule with eslint-plugin/require-meta-docs-description. Why?

  1. The replacement rule has identical behavior.
  2. The replacement rule is more generic.
  3. The replacement rule can be used by any eslint plugin.
  4. This allows us to reduce the amount of code in eslint.

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

Feel free to double-check that the replacement rule has the same behavior as the deleted rule.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 9, 2020
@bmish bmish changed the title Replace internal-rules/consistent-docs-description with eslint-plugin/require-meta-docs-description Chore: Replace internal-rules/consistent-docs-description with eslint-plugin/require-meta-docs-description Jan 9, 2020
@bmish bmish force-pushed the eslint-plugin-require-meta-docs-description branch from 2b12661 to 7d172da Compare January 9, 2020 05:59
@bmish bmish changed the title Chore: Replace internal-rules/consistent-docs-description with eslint-plugin/require-meta-docs-description Chore: Adopt eslint-plugin/require-meta-docs-description internally Jan 9, 2020
@bmish bmish force-pushed the eslint-plugin-require-meta-docs-description branch from 7d172da to 54cf0e0 Compare January 9, 2020 06:02
@kaicataldo
Copy link
Member

I'm not sure this is the right course of action since I could see us wanting to reserve the right to easily validate internal rules differently than custom plugin rules.

@kaicataldo kaicataldo added chore This change is not user-facing evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jan 9, 2020
@bmish
Copy link
Sponsor Member Author

bmish commented Jan 9, 2020

@kaicataldo I hear that. But I would note that:

  1. eslint is already using all of the other recommended rules from eslint-plugin-eslint-plugin (@not-an-aardvark added it originally)
  2. I specifically wrote eslint-plugin/require-meta-docs-description to match eslint's desired behavior
  3. eslint-plugin/require-meta-docs-description provides a pattern option allowing full customization of the regexp that rule descriptions should match

@kaicataldo
Copy link
Member

Thanks for providing more context around that. Your reasoning makes sense to me!

@bmish bmish force-pushed the eslint-plugin-require-meta-docs-description branch from 54cf0e0 to 5735c98 Compare January 17, 2020 07:08
@bmish bmish force-pushed the eslint-plugin-require-meta-docs-description branch from 5735c98 to 3ab3135 Compare January 17, 2020 07:08
Copy link
Member

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

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@platinumazure
Copy link
Member

@kaicataldo Just want to make sure your concerns around merging this have been addressed-- if so, this can probably be merged. Thanks!

@kaicataldo kaicataldo merged commit 10a79a6 into eslint:master Jan 27, 2020
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

montmanu pushed a commit to montmanu/eslint that referenced this pull request Mar 4, 2020
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 27, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants