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

bmish · 2021-05-06T22:52:03Z

Prerequisites checklist

I have read the contributing guidelines.

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

nzakas

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

lib/linter/linter.js

nzakas · 2021-05-07T00:35:39Z

lib/linter/linter.js

+                        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") {


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

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.

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.

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.

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

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

Sure, updated.

tests/lib/rule-tester/rule-tester.js

nzakas

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

btmills

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

btmills

LGTM, thanks!

mdjermanovic

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

docs/developer-guide/working-with-rules.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic

LGTM, thanks!

* 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)

eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label May 6, 2021

bmish mentioned this pull request May 6, 2021

Reevaluate the meta.docs.suggestion property #14312

Closed

nzakas added breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels May 7, 2021

nzakas requested changes May 7, 2021

View reviewed changes

bmish force-pushed the meta.hasSuggestions branch from a4090cf to f7c476e Compare May 7, 2021 01:03

Breaking: Require meta.hasSuggestions for rules with suggestions

7aaa0fc

bmish force-pushed the meta.hasSuggestions branch from f7c476e to 7aaa0fc Compare May 12, 2021 00:48

nzakas approved these changes May 18, 2021

View reviewed changes

btmills reviewed May 19, 2021

View reviewed changes

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

Docs: add another note about hasSuggestions

5a90aa7

btmills approved these changes May 20, 2021

View reviewed changes

mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 26, 2021

mdjermanovic reviewed May 31, 2021

View reviewed changes

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 12:10

Update docs/developer-guide/working-with-rules.md

9d493bf

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update docs/developer-guide/working-with-rules.md

b731a96

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

btmills mentioned this pull request May 31, 2021

Breaking: require meta for fixable rules (fixes #13349) #14634

Merged

1 task

mdjermanovic approved these changes Jun 6, 2021

View reviewed changes

Merge branch 'master' into meta.hasSuggestions

0fa4fed

bmish added 5 commits July 26, 2021 20:37

enable eslint-plugin/require-meta-has-suggestions rule internally

0d6fa90

Merge branch 'master' into meta.hasSuggestions

297ba5f

update property in Makefile

02eda13

Merge branch 'master' into meta.hasSuggestions

3ecadd8

nzakas merged commit d6a761f into eslint:master Aug 5, 2021

MichaelDeBoey mentioned this pull request Sep 4, 2021

Support ESLint 8.x testing-library/eslint-plugin-testing-library#462

Closed

2 tasks

rafaelss95 mentioned this pull request Oct 12, 2021

ESLint 8 support timdeschryver/eslint-plugin-ngrx#258

Closed

4 tasks

mtomcal mentioned this pull request Oct 14, 2021

chore(deps): update dependency eslint to v8 zendeskgarden/eslint-config#173

Merged

1 task

eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022

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 Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bmish commented May 6, 2021 •

edited

nzakas left a comment

nzakas May 7, 2021

bmish May 7, 2021

nzakas May 11, 2021

bmish May 11, 2021

nzakas May 12, 2021

nzakas May 12, 2021

bmish May 12, 2021

nzakas left a comment

btmills left a comment

btmills left a comment

mdjermanovic left a comment

mdjermanovic left a comment

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

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

Conversation

bmish commented May 6, 2021 • edited

Prerequisites checklist

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

What changes did you make? (Give an overview)

nzakas left a comment

Choose a reason for hiding this comment

nzakas May 7, 2021

Choose a reason for hiding this comment

bmish May 7, 2021

Choose a reason for hiding this comment

nzakas May 11, 2021

Choose a reason for hiding this comment

bmish May 11, 2021

Choose a reason for hiding this comment

nzakas May 12, 2021

Choose a reason for hiding this comment

nzakas May 12, 2021

Choose a reason for hiding this comment

bmish May 12, 2021

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

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

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

bmish commented May 6, 2021 •

edited