docs: Expand rule options schema docs #17198
Conversation
|
|
|
Hi @matwilko!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
matwilko
changed the title
|
Hi @matwilko, thanks for the suggestion. Please sign the CLA. Thanks. |
No problem, I've already started the process, but I need to go through the corporate contributor path so it might take a little while. Any feedback in the meantime would be appreciated :) |
mdjermanovic
added
the
accepted
docs/src/extend/custom-rules.md
Outdated
| @@ -645,6 +668,66 @@ module.exports = { | |||
| }; | |||
| ``` | |||
|
|
|||
| And as an example for the full JSON Schema object form: | |||
There was a problem hiding this comment.
This sounds like the following will be the same schema, just in the JSON Schema object form, but it isn't the same. Perhaps it would be better to really show how the schema for yoda rule options would look like in the JSON Schema object form?
There was a problem hiding this comment.
Sure, I was trying to show that there's more/different stuff you can do with the object schema, but I can see how it looks confusing.
I can split it into two examples, one explicitly showing the array-equivalent object schema, and a second showing off how the object schema can diverge in behaviour?
There was a problem hiding this comment.
I've provided a 1-to-1 map from the array to object form, and then a second more complicated object form separately to demonstrate some of the extra power available.
No, I think the clarifications are very helpful, thanks! |
docs/src/extend/custom-rules.md
Outdated
| @@ -614,17 +614,40 @@ You can also access comments through many of `sourceCode`'s methods using the `i | |||
|
|
|||
| Rules may export a `schema` property, which is a [JSON Schema](https://json-schema.org/) format description of a rule's options which will be used by ESLint to validate configuration options and prevent invalid or unexpected inputs before they are passed to the rule in `context.options`. | |||
|
|
|||
| There are two formats for a rule's exported `schema`: | |||
| If `schema` is not specified, then any options (besides severity) supplied to the rule are passed on verbatim without validation. | |||
There was a problem hiding this comment.
Just want to call out that some of this behavior will be changing in the next major release v9 due to RFC-85 (draft PR).
- Requiring rules with options to specify schemas (removing support for rules with options that are missing schemas)
- Requiring rule implementations to use the object-style format (removing support for the deprecated function-style format)
It would ideal if we can take that into account here by:
- Avoid providing recommendations that conflict with the upcoming requirements
- Write the documentation in such a way that it can be easily updated when the new requirements come into effect
There was a problem hiding this comment.
Avoid providing recommendations that conflict with the upcoming requirements
I don't think the docs as written now recommend not providing a schema, it just explains the consequence of not providing one. I can update the line to note that it is explicitly not recommended, and that this option will be removed in v9. Once v9 drops, the change to the docs should be as simple as replacing that paragraph to explain that if you don't provide a schema object, your rule will not be permitted to have any options specified (equivalent to [])?
Requiring rule implementations to use the object-style format (removing support for the deprecated function-style format)
I don't think these docs have any conflict with that? They explain schema/meta.schema, but that change is eliminating the const rule = function(context, options) { ... }; rule.schema = { ... }; form afaict, which is no longer documented at all?
There was a problem hiding this comment.
Updated docs to:
If
schemais not specified, then any options (besides severity) supplied to the rule are passed on verbatim without validation. This is a legacy behaviour though, and all rules are encouraged to specify a schema to aid developers when configuring your rules. Note that in v9, this behaviour will be deprecated entirely, and not specifyingschemawill force the rule to have no options (equivalent to[]) - see the Require schemas and object-style rules RFC.
Does this cover the bases you wanted to cover?
There was a problem hiding this comment.
Also noting in the PR you linked:
Throw with no-op schema schema: {}
Would it be helpful to add some further rule schema validations to catch obvious cases that will always fail, like a top-level type: 'object/number/string'? Could also recurse into anyOf/oneOf/allOf cases and do the same check - i.e. the first type to be tested always needs to be an array?
Not necessarily suggesting adding those validations in this PR, though I'm happy to take a stab at it separately?
matwilko
force-pushed
the
clarify-rule-schemas
branch
from
9caec8b
to
449ffba
Compare
|
Added a note that the supported JSON Schema version is Draft-04 and that later additions like Also updated the jsdoc of |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
My understanding is that currently all new doc updates are paused until #17225 is completed, so I'll leave the PR open but effectively paused for now, and rebase after the doc freeze is done. |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Waiting for the doc freeze to end. |
|
#17225 has been closed. |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
There was a problem hiding this comment.
Thanks for putting this together. I left a bunch of suggestions to clean up the verbiage of the documentation and make it a bit clearer.
I also agree with @mdjermanovic about the code changes, so please take a look at that.
You can optionally remove the code changes and we can deal with the documentation only in this PR and you can resubmit the code changes as a separate PR.
docs/src/extend/custom-rules.md
Outdated
| @@ -614,37 +614,161 @@ You can also access comments through many of `sourceCode`'s methods using the `i | |||
|
|
|||
| Rules may export a `schema` property, which is a [JSON Schema](https://json-schema.org/) format description of a rule's options which will be used by ESLint to validate configuration options and prevent invalid or unexpected inputs before they are passed to the rule in `context.options`. | |||
|
|
|||
| There are two formats for a rule's exported `schema`: | |||
| If `schema` is not specified, then any options (besides severity) supplied to the rule are passed on verbatim without validation. This is a legacy behaviour though, and all rules are encouraged to specify a schema to aid developers when configuring your rules. Note that in v9, this behaviour will be deprecated entirely, and not specifying `schema` will force the rule to have no options (equivalent to `[]`) - see the [Require schemas and object-style rules](https://github.com/eslint/rfcs/blob/main/designs/2021-schema-object-rules/README.md) RFC. | |||
There was a problem hiding this comment.
| If `schema` is not specified, then any options (besides severity) supplied to the rule are passed on verbatim without validation. This is a legacy behaviour though, and all rules are encouraged to specify a schema to aid developers when configuring your rules. Note that in v9, this behaviour will be deprecated entirely, and not specifying `schema` will force the rule to have no options (equivalent to `[]`) - see the [Require schemas and object-style rules](https://github.com/eslint/rfcs/blob/main/designs/2021-schema-object-rules/README.md) RFC. | |
| **Note:** Prior to ESLint v9.0.0, rules without a `schema` are passed their options directly from the config without any validation. In ESLint v9.0.0 and later, rules without schemas will throw errors when options are passed. See the [Require schemas and object-style rules RFC](https://github.com/eslint/rfcs/blob/main/designs/2021-schema-object-rules/README.md) for further details. |
There was a problem hiding this comment.
I'm not sure your suggestion quite covers the proposed strictness in the RFC.
I think the way you've phrased it implies that even if a user specifies options for a rule without a schema, they will be ignored and an empty array passed. But the RFC proposes something stricter, i.e. it is an error for a user to specify any options if a rule doesn't provide a schema.
I know the original version doesn't explicitly spell out this consequence either - should we perhaps highlight the consequence of this breaking change for existing schema-less rules? Perhaps the following:
Note: Prior to ESLint v9.0.0, rules without a
schemaare passed their options directly from the config without any validation. In ESLint v9.0.0 and later, rules without schemas will be treated as if they specified a schema of[]- this will cause errors for users that specify options for your rule when they upgrade to ESLint v9.0.0+. See the Require schemas and object-style rules RFC for further details.
There was a problem hiding this comment.
Ah good catch! I've updated my suggestion above to better match reality.
@bmish can you double-check that my suggested change above matches with your intent?
There was a problem hiding this comment.
Yep, even more short and sweet. Will merge in now - I'm just making these changes directly on the branch rather than committing the suggestions in the GH UI 😄
There was a problem hiding this comment.
That's fine, just make sure the conversations are marked as "Resolved" so we know what's been done.
There was a problem hiding this comment.
Sorry late reply but it looks good.
docs/src/extend/custom-rules.md
Outdated
| There are two formats for a rule's exported `schema`: | ||
| If `schema` is not specified, then any options (besides severity) supplied to the rule are passed on verbatim without validation. This is a legacy behaviour though, and all rules are encouraged to specify a schema to aid developers when configuring your rules. Note that in v9, this behaviour will be deprecated entirely, and not specifying `schema` will force the rule to have no options (equivalent to `[]`) - see the [Require schemas and object-style rules](https://github.com/eslint/rfcs/blob/main/designs/2021-schema-object-rules/README.md) RFC. | ||
|
|
||
| When validating a rule's options, there are 3 main preparation/validation steps: |
There was a problem hiding this comment.
| When validating a rule's options, there are 3 main preparation/validation steps: | |
| When validating a rule's options, there are three steps: |
docs/src/extend/custom-rules.md
Outdated
| 1. If the options are not an array, the value is wrapped into an array (e.g. `"off"` becomes `["off"]`) | ||
| 2. ESLint will validate the first element of the options array as a severity (`"off"`, `"warn"`, `"error"`, `0`, `1`, `2`) | ||
| 3. If the rule is enabled, then the remaining elements of the array are sliced and the rule's schema validation is run on this sliced options array (e.g. `["warn", "never", { someOption: 5 }]` results in the schema being applied to `["never", { someOption: 5 }]`) |
There was a problem hiding this comment.
You are mixing up terms here. "options" means anything passed in the config except the severity, so we need to clean this up a bit.
| 1. If the options are not an array, the value is wrapped into an array (e.g. `"off"` becomes `["off"]`) | |
| 2. ESLint will validate the first element of the options array as a severity (`"off"`, `"warn"`, `"error"`, `0`, `1`, `2`) | |
| 3. If the rule is enabled, then the remaining elements of the array are sliced and the rule's schema validation is run on this sliced options array (e.g. `["warn", "never", { someOption: 5 }]` results in the schema being applied to `["never", { someOption: 5 }]`) | |
| 1. If the rule config is not an array, then the value is wrapped into an array (e.g. `"off"` becomes `["off"]`); if the rule config is an array then it is used directly. | |
| 2. ESLint validates the first element of the rule config array as a severity (`"off"`, `"warn"`, `"error"`, `0`, `1`, `2`). | |
| 3. If the rule is enabled, then the other elements of the array (everything except the severity) are copied an options array (into the `context.options` property) and the rule's schema validation is run on this array (e.g. `["warn", "never", { someOption: 5 }]` results in the schema being applied to `["never", { someOption: 5 }]`). |
There was a problem hiding this comment.
Sure, I was trying to find a consistent way to refer to the so-close-but-ever-so-slightly-different concepts 😄 Wasn't super-happy with "sliced options array" myself, but didn't find a better way to very specifically refer to it - more than happy to go with referring to context.options directly throughout.
Looking at this again, we don't actually say what happens if the rule is considered disabled, so I'm going to add another step after step 2:
- If the severity is
offor0, then the rule is disabled and validation stops, ignoring any other elements of the rule config array.
Appreciate the edits on the last bullet - I think we can go another step further and make it even clearer by splitting it into two bullets explaining copying the options elements into context.options, and then separately running the schema validation on context.options.
docs/src/extend/custom-rules.md
Outdated
|
|
||
| In both cases, these should exclude the [severity](../use/configure/rules#rule-severities), as ESLint automatically validates this first. | ||
| Note that this means that the rule schema cannot see or validate the severity, it only sees the array elements _after_ the severity. |
There was a problem hiding this comment.
| Note that this means that the rule schema cannot see or validate the severity, it only sees the array elements _after_ the severity. | |
| Note: this means the rule schema cannot validate the severity., The rule schema only validates the array elements _after_ the severity in a rule config. |
There was a problem hiding this comment.
Nice. I'm also going to add an extra sentence on the end to really spell it out:
There is no way for a rule to know what severity it is configured at.
docs/src/extend/custom-rules.md
Outdated
| }; | ||
| ``` | ||
|
|
||
| Object schemas are more powerful though, and can be much more precise and restrictive in what is permitted. For example, the below schema always requires the first option to be specifed (a number between 0 and 10), but the second option is optional, and can either be an object with some options explicitly set, or `"off"` or `"strict"`. |
There was a problem hiding this comment.
| Object schemas are more powerful though, and can be much more precise and restrictive in what is permitted. For example, the below schema always requires the first option to be specifed (a number between 0 and 10), but the second option is optional, and can either be an object with some options explicitly set, or `"off"` or `"strict"`. | |
| Object schemas can be more precise and restrictive in what is permitted. For example, the below schema always requires the first option to be specified (a number between 0 and 10), but the second option is optional, and can either be an object with some options explicitly set, or `"off"` or `"strict"`. |
There was a problem hiding this comment.
I've also updated the example code to not use yoda as the rule name, to avoid any confusion with the real rule.
docs/src/extend/custom-rules.md
Outdated
| } | ||
| ``` | ||
|
|
||
| And note the easy pitfall of specifying a schema that will _always_ fail because it doesn't account for the array being validated: |
There was a problem hiding this comment.
| And note the easy pitfall of specifying a schema that will _always_ fail because it doesn't account for the array being validated: | |
| Remember, rule options are always an array, so be careful not to specify a schema for a non-array type at the top level. Here's an example schema that will always fail validation: | |
| And note the easy pitfall of specifying a schema that will always fail because it doesn't account for the array being validated: |
There was a problem hiding this comment.
I've also added another sentence to really spell out the consequence of doing this wrong:
If your schema does not specify an array at the top-level, users can never enable your rule, as their configuration will always be invalid when the rule is enabled.
matwilko
force-pushed
the
clarify-rule-schemas
branch
from
449ffba
to
f34137e
Compare
|
Hi @matwilko!, thanks for the Pull Request The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by running Read more about contributing to ESLint here |
|
Thanks for the feedback @nzakas, much appreciated, and definitely some improvement there 😄 I've removed the added tests, so this is now just a docs update. I'll move the new tests into a separate PR, and I'm also going to refactor the rule schema fetching code a little so that I've highlighted any changes I've made on top of your suggestions inline above. I also added a note to the JSON Schema Draft-04 bullet to highlight that moving to a later spec is explicitly planned to not happen. |
matwilko
force-pushed
the
clarify-rule-schemas
branch
from
f34137e
to
52e90c5
Compare
matwilko
force-pushed
the
clarify-rule-schemas
branch
from
52e90c5
to
ad5311f
Compare
matwilko
changed the title
* main: (101 commits) docs: Migrating `eslint-env` configuration comments (eslint#17390) Merge pull request from GHSA-qwh7-v8hg-w8rh feat: adds option for allowing empty object patterns as parameter (eslint#17365) fix: FlatESLint#getRulesMetaForResults shouldn't throw on unknown rules (eslint#17393) docs: fix Ignoring Files section in config migration guide (eslint#17392) docs: Update README feat: Improve config error messages (eslint#17385) fix: Update no-loop-func to not overlap with no-undef (eslint#17358) docs: Update README docs: Update README 8.45.0 Build: changelog update for 8.45.0 chore: package.json update for @eslint/js release docs: add playground links to correct and incorrect code blocks (eslint#17306) docs: Expand rule option schema docs (eslint#17198) docs: Config Migration Guide (eslint#17230) docs: Update README fix: Fix suggestion message in `no-useless-escape` (eslint#17339) docs: Update README chore: update eslint-config-eslint exports (eslint#17336) ...
eslint-github-bot
bot
added
the
archived due to age
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[] Other, please explain:
What changes did you make? (Give an overview)
While reading the documentation on how rules are configured and specifying options schemas on custom rules, it wasn't clear to me exactly how the two kinds of schema values (object/array) were applied, and I spent a few hours tearing my hair out trying to make a custom object schema work before delving into the code and realising I was supposed to be validating an array!
RuleValidator.validateandgetRuleOptionsSchemawere simple enough to understand, but the documentation doesn't really spell out some of the details of how the validation works and the schema(s) are applied. I've tried to expand the documentation substantially in this area to try to make the validation process clearer and highlight some implied consequences and pitfalls. I've also expanded the provided examples to show both valid/invalid potential configurations for the example schema, and added an object schema example, along with an invalid example to highlight the pitfall I fell into.Is there anything you'd like reviewers to focus on?
Not sure if the expanded docs are perhaps too verbose? I was trying to err on the side of spelling things out rather than leave out important details.