From 41598f3a0d81f56ae6c75f59273269a54db50085 Mon Sep 17 00:00:00 2001 From: Samuel Therrien Date: Tue, 22 Oct 2024 13:50:04 -0400 Subject: [PATCH 1/5] 2024-per-rule-autofix-configuration initial commit --- .../README.md | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 designs/2024-per-rule-autofix-configuration/README.md diff --git a/designs/2024-per-rule-autofix-configuration/README.md b/designs/2024-per-rule-autofix-configuration/README.md new file mode 100644 index 00000000..79f2cbeb --- /dev/null +++ b/designs/2024-per-rule-autofix-configuration/README.md @@ -0,0 +1,159 @@ +- Repo: +- Start Date: 2024-10-22 +- RFC PR: +- Authors: [Samuel Therrien](https://github.com/Samuel-Therrien-Beslogic) (aka [@Avasam](https://github.com/Avasam)) + +# Per-rule autofix configuration + +## Summary + + +This feature aims to make it possible to control autofixes through shareable configuration on a per-rule basis. + +## Motivation + + +Some rules provide autofixing, which is great, but can sometimes be broken or otherwise simply unwanted for various reasons. +Unsafe autofixes should be suggestions, and broken fixes should be reported, *but* ESLint is a large ecosystem where some very useful plugins are not always actively maintained. Even then, wanting to disable an autofix for project-specific or personal reasons could still happen. + +## Detailed Design + + + +Similar to how Ruff () does it, a top-level key to specify which rules to not autofix would be in my opinion the least disruptive and forward/backwards compatible. It should be overridable (in the `overrides` section), and picked up when extending a configuration. + +Concretely, it could look like this: + +```js +export default [ + { + autofixes: { + // We don't want this to autofix, as a rule suddenly not failing should require human attention + "@eslint-community/eslint-comments/no-unused-disable": false, + }, + rules: { + '@eslint-community/eslint-comments/no-unused-disable': 'error', + } + overrides: [ + files: ["*.spec.js"], + autofixes: { + // Let's pretend we want this to be autofixed in tests, for the sake of the RFC + "@eslint-community/eslint-comments/no-unused-disable": true, + }, + ] + } +] +``` + +I think that disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like disabling a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. +It's still an open question whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. + +## Documentation + + +I think that "Configuring autofixes" or "Disabling autofixes" could be documented as a subsection of [Configuring Rules](https://eslint.org/docs/latest/use/configure/rules). Or as a section on the same level (between "Configuring Rules" and "Configuring Plugins") + +## Drawbacks + + +A potential drawback I could see is that the configuration for autofixing a rule is not directly related with the rule itself. As a counter, I'd say this is already the case for plenty of rule-related settings, environment and parser configurations, etc. It's also less of a drawback than [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself). + +## Backwards Compatibility Analysis + + +Given that this proposal adds a new optional configuration section, this feature should be fully backwards compatible. Users that don't want to use this feature should stay completely unaffected. (see [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself)) + +## Alternatives + + + +### Configure in the rule itself + +Another approach I can think of is to encode that in the rule config itself. Something like `"my-plugin/my-rule": "[{severity: "error", autofix: False}, {...otherConfigs}]"` but it's harder to commit to such a change, and means that any config extension needs to reconfigure the rule correctly just to disable autofixing (which is already an issue when someone wants to set a pre-configured rule as warning for example) + +### Use of a 3rd-party plugin + + is a tool that exists to currently work around this limitation of ESLint, but it is not perfect. + +1. It is an extra third-party dependency, with its own potential maintenance issues (having to keep up with ESLint, separate dependencies that can fall out of date, obsolete, unsecure, etc.) +2. It may not work in all environments. For example, pre-commit.ci: +3. It may not work correctly with all third-party rules: + +## Open Questions + + +- Where exactly should the documentation go ? +- Should the value be more than a boolean ? (for example if we want to affect offering suggestions in editors) +- What should the key for the new configuration be ? +- What happens if we mark a rule as "should be autofixed" but there's no fix available? Warn? Silently ignore? +- Whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. + +## Help Needed + + +My knowledge of ESLint's internals isn't that great. Whilst I think it's above the average user due to reading and configuring a lot, I haven't yet even learned how to write a plugin, and haven't migrated any project to ESLint 9 yet. +My free time both at work and personal, is currently also very limited (see how long it too me to just get to writing this RFC). +So I unfortunately don't think I can implement this feature myself, due to both a lack of time, personal motivation (I won't be able to use it for a while, but will push us towards ESLint 9 once implemented), and experience. + +## Frequently Asked Questions + + + +## Related Discussions + + + From 7497a524b5e04ded495d59aef22f33e6622d3e2d Mon Sep 17 00:00:00 2001 From: Samuel Therrien Date: Tue, 22 Oct 2024 14:42:48 -0400 Subject: [PATCH 2/5] Clarify that suggestions should not be disabled. Autofixes are "demoted" to suggestions. --- designs/2024-per-rule-autofix-configuration/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/designs/2024-per-rule-autofix-configuration/README.md b/designs/2024-per-rule-autofix-configuration/README.md index 79f2cbeb..8876ee50 100644 --- a/designs/2024-per-rule-autofix-configuration/README.md +++ b/designs/2024-per-rule-autofix-configuration/README.md @@ -53,6 +53,8 @@ export default [ ] ``` +The fix should still exist as a suggestion. Only autofixing (when running `eslint --fix` or editor action on save) should be disabled. + I think that disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like disabling a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. It's still an open question whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. @@ -121,7 +123,6 @@ Another approach I can think of is to encode that in the rule config itself. Som you can remove this section. --> - Where exactly should the documentation go ? -- Should the value be more than a boolean ? (for example if we want to affect offering suggestions in editors) - What should the key for the new configuration be ? - What happens if we mark a rule as "should be autofixed" but there's no fix available? Warn? Silently ignore? - Whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. From ef18a67e2ff80b213f21910bca81cdb7ce662010 Mon Sep 17 00:00:00 2001 From: Samuel Therrien Date: Thu, 24 Oct 2024 11:19:02 -0400 Subject: [PATCH 3/5] Update configuration example, addressing michaelfaith comments --- .../README.md | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/designs/2024-per-rule-autofix-configuration/README.md b/designs/2024-per-rule-autofix-configuration/README.md index 8876ee50..9547c48d 100644 --- a/designs/2024-per-rule-autofix-configuration/README.md +++ b/designs/2024-per-rule-autofix-configuration/README.md @@ -28,35 +28,34 @@ Unsafe autofixes should be suggestions, and broken fixes should be reported, *bu used. Be sure to define any new terms in this section. --> -Similar to how Ruff () does it, a top-level key to specify which rules to not autofix would be in my opinion the least disruptive and forward/backwards compatible. It should be overridable (in the `overrides` section), and picked up when extending a configuration. +Similar to how Ruff () does it, a top-level key to specify which rules to not autofix would be in my opinion the least disruptive and forward/backwards compatible. It should be overridable in per-file configurations, and picked up when extending a configuration. Concretely, it could look like this: ```js export default [ { - autofixes: { + disableAutofixes: { // We don't want this to autofix, as a rule suddenly not failing should require human attention - "@eslint-community/eslint-comments/no-unused-disable": false, + "@eslint-community/eslint-comments/no-unused-disable": true, }, rules: { '@eslint-community/eslint-comments/no-unused-disable': 'error', } - overrides: [ - files: ["*.spec.js"], - autofixes: { - // Let's pretend we want this to be autofixed in tests, for the sake of the RFC - "@eslint-community/eslint-comments/no-unused-disable": true, - }, - ] - } + }, + { + files: ["*.spec.js"], + disableAutofixes: { + // Let's pretend we want this to be autofixed in tests, for the sake of the RFC + "@eslint-community/eslint-comments/no-unused-disable": false, + }, + }, ] ``` The fix should still exist as a suggestion. Only autofixing (when running `eslint --fix` or editor action on save) should be disabled. -I think that disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like disabling a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. -It's still an open question whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. +The chosen key name `disableAutofixes` aims to remove the concern about "turning on" an autofix that doesn't exist. Disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like turning `off` a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. ## Documentation @@ -123,9 +122,6 @@ Another approach I can think of is to encode that in the rule config itself. Som you can remove this section. --> - Where exactly should the documentation go ? -- What should the key for the new configuration be ? -- What happens if we mark a rule as "should be autofixed" but there's no fix available? Warn? Silently ignore? -- Whether *enabling* autofixes for a rule that doesn't exist should warn, error or be silent. ## Help Needed @@ -157,4 +153,6 @@ So I unfortunately don't think I can implement this feature myself, due to both If there is an issue, pull request, or other URL that provides useful context for this proposal, please include those links here. --> - +- +- +- From e4440f9cfe20e1b192e898a3753d87f4331dc3d7 Mon Sep 17 00:00:00 2001 From: Samuel Therrien Date: Thu, 24 Oct 2024 11:28:37 -0400 Subject: [PATCH 4/5] Add FAQ entry about array vs record --- designs/2024-per-rule-autofix-configuration/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/designs/2024-per-rule-autofix-configuration/README.md b/designs/2024-per-rule-autofix-configuration/README.md index 9547c48d..9a2f9fcf 100644 --- a/designs/2024-per-rule-autofix-configuration/README.md +++ b/designs/2024-per-rule-autofix-configuration/README.md @@ -145,6 +145,9 @@ So I unfortunately don't think I can implement this feature myself, due to both in this section. --> +Q: Could `disableAutofixes` be an array of autofixes to disable? +A: `disableAutofixes` is a record to allow re-enabling autofixes in downstream configurations and on a per-file basis. We could allow a shorthand to `disableAutofixes` to accept an array of rules to disable the autofix for, but that would result in additional complexity on the implementation side with marginal benefits to the user. + ## Related Discussions I think that "Configuring autofixes" or "Disabling autofixes" could be documented as a subsection of [Configuring Rules](https://eslint.org/docs/latest/use/configure/rules). Or as a section on the same level (between "Configuring Rules" and "Configuring Plugins") +As a new top-level property added to configuration objects, `disableAutofixes` should be documented in [Configuration Files > Configuration Objects](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-objects) section. Additionally we may want to add a note to [Custom Rules](https://eslint.org/docs/latest/extend/custom-rulesd) to mention that some autofixes will be converted automatically into suggestions when the new feature is used. + ## Drawbacks - Where exactly should the documentation go ? +- Where this needs to be implemented in code. Those familiar with ESLint's codebase are welcome to provide this information ## Help Needed