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

Breaking: Require meta.hasSuggestions for rules with suggestions #14573

Merged
merged 10 commits into from Aug 5, 2021

Conversation

@bmish
Copy link
Sponsor Member

@bmish bmish commented May 6, 2021

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

Fixes #14312.

  • Renames the old meta.docs.suggestion property to meta.hasSuggestions to indicate whether a rule provides suggestions (improved clarity and alignment with the meta.fixable property which is mandatory for fixable rules)
  • Enforces the presence of the new property for rules that provide suggestions (the presence of the old property was not enforced)
  • Enforces that the old property is no longer provided
  • Updates core rules that provide suggestions to use the new property
  • Adds the new property to test cases and documentation (some of which were missing it before)

This ensures we have a clear and reliable property to use for both humans and tooling to determine whether a rule provides suggestions.

Before merging:

  • Double-check that all core rules with suggestions have been updated with the new property
Copy link
Member

@nzakas nzakas left a comment

This looks really good. Just a couple small suggestions. Nice work.

lib/linter/linter.js Outdated Show resolved Hide resolved
if (problem.suggestions && !(rule.meta && rule.meta.hasSuggestions === true)) {
throw new Error("Rules with suggestions should set the `meta.hasSuggestions` property to `true`.");
}
if (rule.meta && rule.meta.docs && typeof rule.meta.docs.suggestion !== "undefined") {
Copy link
Member

@nzakas nzakas May 7, 2021

Choose a reason for hiding this comment

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

I don’t think we need this as this property is already ignored. The check on line 925 covers the most important case.

Copy link
Member Author

@bmish bmish May 7, 2021

Choose a reason for hiding this comment

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

I'm worried that if we don't enforce the absence of the old, obsolete property, then we'll end up with the confusing and messy situation where some rules have both the new property (meta.hasSuggestions) and the old property (meta.docs.suggestion). This will happen because plugin developers performing the migration to ESLint 8 may only see the exception Rules with suggestions should set the meta.hasSuggestions property to true and not realize or forget that they're also supposed to cleanup/remove the old property. Furthermore, if some rules still have the old property, then some people/tooling may still try to rely on it.

Disallowing the old property also fits with the spirit of many of the ESLint 6 and ESLint 7 changes to ensure that configurations/tests are validated more strictly and do not contain unknown properties. There's a separate question of whether we should apply stricter to the meta object in general (I'm in favor of stricter validation wherever possible and would like to pursue this if there's interest), but at the least I hope we disallow this obviously-outdated property.

Copy link
Member

@nzakas nzakas May 11, 2021

Choose a reason for hiding this comment

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

My point is that it doesn’t matter if people have meta.Docs.suggestions because that value is never used by us. If they are using it for some reason, they can keep using it.

People can put whatever they want in meta.Docs as it’s just designed to hold documentation-type metadata. I can’t think of a reason why we would want to throw an error for anything in that section. People are free to use stuff in there as they see fit.

Copy link
Member Author

@bmish bmish May 11, 2021

Choose a reason for hiding this comment

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

Okay sounds good. I have another idea about improving the migration behavior.

  • If a rule is missing meta.hasSuggestions and also has meta.docs.suggestion, throw an error saying:

    Please migrate from meta.docs.suggestion to the replacement property meta.hasSuggestions.

  • If a rule is just missing meta.hasSuggestions, throw an error:

    Rules with suggestions must set the meta.hasSuggestions property to true.

This way, people can use the meta.docs object freely with no restrictions, but we would offer a more personalized error message to people performing the ESLint v8 migration from the old property to the new property.

Copy link
Member

@nzakas nzakas May 12, 2021

Choose a reason for hiding this comment

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

Maybe we could change the wording a bit?

Rules with suggestions must set the meta.hasSuggestions property to true. meta.docs.suggestions is ignored by ESLint.

That way it sounds less like you need to remove meta.docs.suggestions and more like, “hey, we aren’t using this value.”

Copy link
Member

@nzakas nzakas May 12, 2021

Choose a reason for hiding this comment

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

To be clear, that would be only for rules with meta.docs.suggestions. Otherwise, the shorter error is fine.

Copy link
Member Author

@bmish bmish May 12, 2021

Choose a reason for hiding this comment

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

Sure, updated.

tests/lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
@bmish bmish force-pushed the meta.hasSuggestions branch from a4090cf to f7c476e May 7, 2021
@bmish bmish force-pushed the meta.hasSuggestions branch from f7c476e to 7aaa0fc May 12, 2021
nzakas
nzakas approved these changes May 18, 2021
Copy link
Member

@nzakas nzakas left a comment

LGTM. @eslint/eslint-tsc looking for another set of eyes.

Copy link
Member

@btmills btmills left a comment

Implementation and tests look good. Thank you for doing this! Nice touch giving a separate exception message with meta.docs.suggestion. Just one request for an extra note in the docs to match how we treat fixable, then LGTM.

docs/developer-guide/working-with-rules.md Show resolved Hide resolved
Copy link
Member

@btmills btmills left a comment

LGTM, thanks!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

I have a couple of suggestions about the documentation. Everything else looks good!

docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved
docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved
bmish and others added 2 commits May 31, 2021
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

bmish added 5 commits Jul 27, 2021
* master:
  Chore: Adopt `eslint-plugin/require-meta-docs-url` rule internally (eslint#14823)
  Docs: New syntax issue template (eslint#14826)
  Chore: assertions on reporting loc in `unicode-bom` (refs eslint#12334) (eslint#14809)
  Docs: fix multiple broken links (eslint#14833)
  Chore: use `actions/setup-node@v2` (eslint#14816)
  Docs: Update README team and sponsors
  7.31.0
  Build: changelog update for 7.31.0
  Upgrade: @eslint/eslintrc to v0.4.3 (eslint#14808)
  Update: add end location to report in `consistent-return` (refs eslint#12334) (eslint#14798)
  Docs: update BUG_REPORT template (eslint#14787)
  Docs: provide more context to no-eq-null (eslint#14801)
@nzakas nzakas merged commit d6a761f into eslint:master Aug 5, 2021
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants