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

Fix: Throw error when rule is missing meta.docs.suggestion property #14292

Conversation

bmish
Copy link
Member

@bmish bmish commented Apr 3, 2021

Prerequisites checklist

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

[ ] Documentation update
[X] 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
[ ] Other, please explain:

Tell us about your environment

  • ESLint Version: 7.23
  • Node Version: 14
  • npm Version: 7.6

What did you do? Please include the actual source code causing the issue.

There is a meta.docs.suggestion property mentioned in the documentation and used in eslint's handleful of rules with suggestions.

suggestion (boolean) specifies whether rules can return suggestions (defaults to false if omitted)

Unfortunately, the usage of this property is not enforced / validated, i.e. a rule providing a suggestion can easily forget to specify the meta.docs.suggestion property. As a result, there is no consequence for forgetting to enable the meta.docs.suggestion property, and this leads to inconsistency as well as this property being rather useless / unreliable.

What did you expect to happen?

eslint should throw an error when a rule provides a suggestion without enabling the meta.docs.suggestion property. This would match the behavior of the meta.fixable which must be specified when a rule provides autofixes.

Having static properties like meta.docs.suggestion or meta.fixable make it easy for humans and tooling to tell whether rules provides autofixes or suggestions. For example, these properties can make it convenient to write tests that rules with autofixes/suggestions mention that they have autofixes/suggestions in their documentation.

What actually happened? Please include the actual, raw output from ESLint.

eslint did not throw an error.

What changes did you make? (Give an overview)

Updates eslint to throw an error when a rule provides a suggestion without enabling the meta.docs.suggestion property.

This is a breaking bug fix / change in that rules (and tests of rules) that provided suggestions without enabling the meta.docs.suggestion property will now fail.

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

No.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Apr 3, 2021
@mdjermanovic mdjermanovic added breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features 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 Apr 5, 2021
Comment on lines +925 to +927
if (problem.suggestions && !(rule.meta && rule.meta.docs && rule.meta.docs.suggestion === true)) {
throw new Error("Rules with suggestions should set the `meta.docs.suggestion` property to `true`.");
}
Copy link
Member

Choose a reason for hiding this comment

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

There are not many details or discussions about this property in the RFC, but it was probably added to docs instead of as a top-level property because it was never meant to be mandatory, even not for rules that provide suggestions. I believe the purpose was only to flag those rules on our website. The whole docs object is optional for third-party rules, so it doesn't look to me that this should throw an error.

fixable is different, its value was supposed to be used for some optimizations.

Copy link
Member Author

@bmish bmish Apr 5, 2021

Choose a reason for hiding this comment

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

I see. Yeah it doesn't seem like there was much discussion about it. I personally would have chosen one of the following different designs:

  1. Add a new boolean property meta.suggestable (which would be a sibling to meta.fixable, and follow the same naming convention as fixable) and make it mandatory when a rule provides suggestions. Eliminate meta.docs.suggestion. This is my ideal design, but it's a bigger API change.
  2. Keep meta.docs.suggestion and make it mandatory when a rule provides suggestions.

As I mentioned in the PR description, the benefits of making the field mandatory include:

  • Having static properties make it easy for both humans and tooling to tell whether rules provides autofixes or suggestions. For example, these properties can make it convenient to write tests that rules with autofixes/suggestions mention that they have autofixes/suggestions in their documentation.
  • The question of whether a rule provides suggestions is a factual question with only one answer, so it's odd we would leave the suggestion property as optional when it would be straightforward for a rule to provide it (unlike the other properties in meta.docs which are more free-form fields).
  • The suggestion property is essentially useless if it's presence is not enforced as neither humans nor tooling can actually rely on it.

Would you see a path forward to making an improvement here? I realize any improvement would be a breaking change, and I would be happy to write an RFC with a proposal.

Copy link
Member

Choose a reason for hiding this comment

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

Anything under “docs” is intended to be optional and used only for generating the website. I don’t think it’s presence or absence should indicate anything.

We have the “fixable” top-level property because we want to know if a rule can apply fixes because we can optimize when rules run. There’s no such concern for suggestions because they don’t make any changes to the source code.

So, I’m not sure we should do anything here except maybe update the documentation to reflect reality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we don't currently have a concrete need for knowing whether rules provide suggestions, I guess I just personally wish that basic rule information like that was conveniently and reliably available to anyone interested to make use of it.

But if we won't consider changing this, then we can close this PR. I opened a separate PR #14298 to extract the documentation improvements :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and opened a PR to create a lint rule called eslint-plugin/require-meta-docs-suggestion for anyone interested to enforce the presence of the meta.docs.suggestion property for suggestable rules.

Copy link
Member

Choose a reason for hiding this comment

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

We can consider changing it, but it needs more discussion. If you want to pursue it, I’d suggest opening an issue to start the discussion as such a change would need an RFC before being implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I opened this issue to discuss: #14312.

@bmish bmish closed this Apr 8, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 6, 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 Oct 6, 2021
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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants