From a99c45634f914e707ca7a6f48ea36eeb36d239c1 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Mon, 22 Apr 2024 09:06:05 +0300 Subject: [PATCH 01/27] Add RFC for baseline support --- designs/2024-baseline-support/README.md | 201 ++++++++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 designs/2024-baseline-support/README.md diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md new file mode 100644 index 00000000..d50cbac9 --- /dev/null +++ b/designs/2024-baseline-support/README.md @@ -0,0 +1,201 @@ +- Repo: eslint/eslint +- Start Date: 2024-04-20 +- RFC PR: (leave this empty, to be filled in later) +- Authors: [Iacovos Constantinou](https://github.com/softius) + +# Introduce baseline system to suppress existing errors + +## Summary + + + +Declare currently reported errors as the "baseline", so that they are not being reported in subsequent runs. It allows developers to enable one or more rules as `error` and be notified when new ones show up. + +## Motivation + + + +Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example, is `no-explicit-any`. Unless the rule is enabled at the early stages of the project, is becoming harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. + +This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address future violations. + +## Detailed Design + + + +The suggested solution introduces the concept of a baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. + +Here is how the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. + +``` +{ + "src/app/components/foobar/foobar.component.ts": { + "@typescript-eslint/no-explicit-any": 1 + } +} +``` + +### Generating the baseline + +A new option `--generate-baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: + +``` bash +eslint --generate-baseline ./src +``` + +The above will go through each result item and rules, and count the number of errors ( `severity == 2` ). If one or more such errors are found, the necessary details must be stored in the baseline file. The process should take place right after the fixes are applied to avoid counting errors that are about to be fixed. + +### Matching against the baseline + +The suggested solution always compares against the baseline, given that the baseline file `.eslint-baseline.json` exists. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. + +This will go through each result item and rules, and check each rule against the baseline. If the file and rule are part of the baseline, we decrease the number of errors to be ignored (in memory), and remove the result item. This needs to take place after the fixes are applied so that we compare the non-fixable issues only. It also needs to take place before the error counting, so that the remaining errors are counted correctly. + +### Maintaining a lean baseline + +When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. + +### Caching + +Caching must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: + +- Generating the baseline can be based on the cache file and should be faster when the cache file is used. +- Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. +- It even allows developers to delete completely the baseline and still take advantage of the cached file in subsequent runs. + +## Documentation + + + +We should update [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface) to document the newly introduced option. A dedicated section should be added in Documentation to explain how baseline works. + +## Drawbacks + + + +The baseline can be generated and used only when linting files. It can not be leveraged when using `stdin` since it relies on file paths. + +## Backwards Compatibility Analysis + + + +If the baseline file is not generated, ESLint CLI behavior will not change. This change is therefore backwards compatible to start with. + +If the baseline file is generated, ESLint CLI will compare the errors against the errors included in the baseline. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the baseline without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the baseline can be easily deleted and cancel the new behavior. + +Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean baseline"). This might be have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. + +## Alternatives + + + +Unfortunately existing approaches do not address the issue at its core and come with their own set of drawbacks. It is worth mentioning that the suggested solution is based on [how baseline works in PHPStan](https://phpstan.org/user-guide/baseline). + +The following sections are extracted from [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) where [@jfmengels](https://github.com/jfmengels) did a detailed analysis about existing approaches and their drawbacks. + +### Using warnings + +This use-case is apparently what the "warn" severity level is for. + +A large problem with warnings is that as soon as there are more than a few warnings, you don't notice new ones showing up. A common practice I've seen quite often is to avoid warnings altogether, and to only use errors to avoid new problems creeping in. But that doesn't solve the problem of all the existing errors. + +Also, users can too easily ignore the new errors, so in a way, the rule is enabled without being enforced when IMO the point of a linter is to enforce rules. + +### Using disable comments + +One can use disable comments to temporarily suppress errors, by adding a comment like `// eslint-disable rule-name -- FIX THIS LATER` + +Disable comments can be used to enable a rule as an error early, by adding them everywhere where an error is currently reported (and that is actually something that can be automated by some linters). + +But disable comments have the tendency to be hard to distinguish from other disable comments created for reasons such as false positives or disagreements on the rule, especially when there is no enforced need to add a message on the comment. Meaning that once you decide to tackle the existing errors, they can be hard to detect (or to distinguish from ones that are disabled for good reasons). + +They also "pollute" the codebase in a way that is quite visible, and makes users numb to the fact of using disable comments. + +### Ignoring parts of the project + +It is also possible to simply disable the rule in each file that is currently reporting errors, either through manually configuring the rule in the ESLint config, or by adding a disable comment at the top of the file that disables the rule for the entire file. + +This has multiple downsides: + +* While new errors are enforced in the other files, new errors can creep in the ignored files +* If/when the errors in the ignored files get removed, the user has to remember to re-enable the rule on this file. Otherwise new errors can creep in also. + + +## Open Questions + + + +None so far. + +## Help Needed + + + +I expect to implement this change. + +## Frequently Asked Questions + + + +No questions so far. + +## Related Discussions + +* [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) +* [PHPStan - The Baseline](https://phpstan.org/user-guide/baseline) + + From f9b39769b586f9961b6e09fe9f6b7376f79fb306 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Mon, 22 Apr 2024 12:05:57 +0300 Subject: [PATCH 02/27] Fix typos --- designs/2024-baseline-support/README.md | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index d50cbac9..c852d893 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -16,7 +16,7 @@ Declare currently reported errors as the "baseline", so that they are not being -Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example, is `no-explicit-any`. Unless the rule is enabled at the early stages of the project, is becoming harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. +Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example, is `no-explicit-any`. Unless the rule is enabled during the early stages of the project, it becomes harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address future violations. @@ -31,9 +31,9 @@ This can be counterintuitive for enabling new rules as `error`, since the develo used. Be sure to define any new terms in this section. --> -The suggested solution introduces the concept of a baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. +To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. -Here is how the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. +Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. ``` { @@ -51,13 +51,15 @@ A new option `--generate-baseline` can be added to ESLint CLI. When provided, th eslint --generate-baseline ./src ``` -The above will go through each result item and rules, and count the number of errors ( `severity == 2` ). If one or more such errors are found, the necessary details must be stored in the baseline file. The process should take place right after the fixes are applied to avoid counting errors that are about to be fixed. +The above will go through each result item and messages, and count the number of errors (`severity == 2`). If one or more such messages are found, the necessary details must be stored in the baseline file. The process should take place right after the fixes are applied to avoid counting errors that are about to be fixed. ### Matching against the baseline The suggested solution always compares against the baseline, given that the baseline file `.eslint-baseline.json` exists. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. -This will go through each result item and rules, and check each rule against the baseline. If the file and rule are part of the baseline, we decrease the number of errors to be ignored (in memory), and remove the result item. This needs to take place after the fixes are applied so that we compare the non-fixable issues only. It also needs to take place before the error counting, so that the remaining errors are counted correctly. +This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. If the file and rule are part of the baseline, means that we can remove and ignore the result message. This needs to take place after the fixes are applied so that we compare the non-fixable issues only. It also needs to take place before the error counting, so that the remaining errors are counted correctly. + +We can also keep track of which errors from baseline were not matched, that is useful for the next section. ### Maintaining a lean baseline @@ -69,7 +71,7 @@ Caching must contain the full list of detected errors, even those matched agains - Generating the baseline can be based on the cache file and should be faster when the cache file is used. - Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. -- It even allows developers to delete completely the baseline and still take advantage of the cached file in subsequent runs. +- It even allows developers to delete the baseline and still take advantage of the cached file in subsequent runs. ## Documentation @@ -107,7 +109,7 @@ If the baseline file is not generated, ESLint CLI behavior will not change. This If the baseline file is generated, ESLint CLI will compare the errors against the errors included in the baseline. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the baseline without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the baseline can be easily deleted and cancel the new behavior. -Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean baseline"). This might be have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. +Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean baseline"). This might have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. ## Alternatives @@ -134,11 +136,11 @@ Also, users can too easily ignore the new errors, so in a way, the rule is enabl One can use disable comments to temporarily suppress errors, by adding a comment like `// eslint-disable rule-name -- FIX THIS LATER` -Disable comments can be used to enable a rule as an error early, by adding them everywhere where an error is currently reported (and that is actually something that can be automated by some linters). +"Disable comments" can be used to enable a rule as an error early, by adding them everywhere where an error is currently reported (and that is actually something that can be automated by some linters). -But disable comments have the tendency to be hard to distinguish from other disable comments created for reasons such as false positives or disagreements on the rule, especially when there is no enforced need to add a message on the comment. Meaning that once you decide to tackle the existing errors, they can be hard to detect (or to distinguish from ones that are disabled for good reasons). +But "disable comments" have the tendency to be hard to distinguish from other "disable comments" created for reasons such as false positives or disagreements on the rule, especially when there is no enforced need to add a message on the comment. Meaning that once you decide to tackle the existing errors, they can be hard to detect (or to distinguish from ones that are disabled for good reasons). -They also "pollute" the codebase in a way that is quite visible, and makes users numb to the fact of using disable comments. +They also "pollute" the codebase in a way that is quite visible, and makes users numb to the fact of using "disable comments". ### Ignoring parts of the project @@ -147,8 +149,7 @@ It is also possible to simply disable the rule in each file that is currently re This has multiple downsides: * While new errors are enforced in the other files, new errors can creep in the ignored files -* If/when the errors in the ignored files get removed, the user has to remember to re-enable the rule on this file. Otherwise new errors can creep in also. - +* If/when the errors in the ignored files get removed, the user has to remember to re-enable the rule on this file. Otherwise new errors can creep in. ## Open Questions From c9a718c5650363fe8f63f5564f8cbf446112db01 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Mon, 6 May 2024 21:31:16 +0300 Subject: [PATCH 03/27] Add question about warnings --- designs/2024-baseline-support/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index c852d893..09f18656 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -187,7 +187,9 @@ I expect to implement this change. in this section. --> -No questions so far. +### Does this count warnings? + +No, we are only counting errors when generating the baseline. Also only errors are considered when checking against the baseline. ## Related Discussions From 4d73750973efd107d3ec0b7cc4dd5b171c693562 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Mon, 6 May 2024 21:33:01 +0300 Subject: [PATCH 04/27] Convert relative to full paths --- designs/2024-baseline-support/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 09f18656..1c2cfd00 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -33,11 +33,11 @@ This can be counterintuitive for enabling new rules as `error`, since the develo To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. -Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. +Here is what the baseline file looks like. This indicates that the file `"/home/user/project/src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. ``` { - "src/app/components/foobar/foobar.component.ts": { + "/home/user/project/src/app/components/foobar/foobar.component.ts": { "@typescript-eslint/no-explicit-any": 1 } } From 9e035f174b5e60d710073d15a5944804ee180e4f Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Mon, 6 May 2024 21:45:08 +0300 Subject: [PATCH 05/27] Replace --generate-baseline with --baseline/-location --- designs/2024-baseline-support/README.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 1c2cfd00..e80b1581 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -45,14 +45,20 @@ Here is what the baseline file looks like. This indicates that the file `"/home/ ### Generating the baseline -A new option `--generate-baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: +A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: ``` bash -eslint --generate-baseline ./src +eslint --baseline ./src ``` The above will go through each result item and messages, and count the number of errors (`severity == 2`). If one or more such messages are found, the necessary details must be stored in the baseline file. The process should take place right after the fixes are applied to avoid counting errors that are about to be fixed. +By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying the file or a directory. If a directory is specified, a baseline file is created inside the specified folder. + +``` bash +eslint --baseline --baseline-location /home/user/project/mycache +``` + ### Matching against the baseline The suggested solution always compares against the baseline, given that the baseline file `.eslint-baseline.json` exists. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. From ed01be1d8d909e4ebe00e143f8699e8f33c2e38c Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Wed, 8 May 2024 19:50:12 +0300 Subject: [PATCH 06/27] Add more implementation details, add relative paths to CWD --- designs/2024-baseline-support/README.md | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index e80b1581..c5b7af38 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -33,11 +33,13 @@ This can be counterintuitive for enabling new rules as `error`, since the develo To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. -Here is what the baseline file looks like. This indicates that the file `"/home/user/project/src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. +Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. + +All paths are relative to CWD. ``` { - "/home/user/project/src/app/components/foobar/foobar.component.ts": { + "src/app/components/foobar/foobar.component.ts": { "@typescript-eslint/no-explicit-any": 1 } } @@ -45,25 +47,31 @@ Here is what the baseline file looks like. This indicates that the file `"/home/ ### Generating the baseline -A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: +A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslintbaseline`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: ``` bash eslint --baseline ./src ``` -The above will go through each result item and messages, and count the number of errors (`severity == 2`). If one or more such messages are found, the necessary details must be stored in the baseline file. The process should take place right after the fixes are applied to avoid counting errors that are about to be fixed. +The above goes through each result item and messages, and counts the number of errors (`severity == 2`). If one or more such messages are found, the necessary details are stored in the baseline file. Note that the file is a relative path to CWD. -By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying the file or a directory. If a directory is specified, a baseline file is created inside the specified folder. +By default, the baseline file is saved at `.eslintbaseline` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. ``` bash eslint --baseline --baseline-location /home/user/project/mycache ``` +To implement this, we will need to add the two new options in `default-cli-options.js`, adjust the config for optionator and add the two new options as comments and arguments for both eslint and cli-engine. Documentation must be updated as well to explain the newly introduced options. + +On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - the method must be called only and only if `--baseline` was provided and set to true. + ### Matching against the baseline -The suggested solution always compares against the baseline, given that the baseline file `.eslint-baseline.json` exists. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. +The suggested solution always compares against the baseline, given that the baseline file exists. By default the baseline file is picked up from `.eslintbaseline`, unless the option `--baseline-location` is provided. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. As mentioned above, `--baseline-location` is a string argument specifying where the baseline was previously saved. -This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. If the file and rule are part of the baseline, means that we can remove and ignore the result message. This needs to take place after the fixes are applied so that we compare the non-fixable issues only. It also needs to take place before the error counting, so that the remaining errors are counted correctly. +This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. + +To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslintbaseline`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. A new method `applyBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - this must be called only and only if the baseline file exists. We can also keep track of which errors from baseline were not matched, that is useful for the next section. @@ -71,6 +79,8 @@ We can also keep track of which errors from baseline were not matched, that is u When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. +To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In `cli.js` we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose more we can display the list of errors that were left unmatched. + ### Caching Caching must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: From d5411af1eb7edd8a1fce5567dd781c3cf19d020b Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Thu, 30 May 2024 18:39:04 +0300 Subject: [PATCH 07/27] Update designs/2024-baseline-support/README.md Co-authored-by: Nicholas C. Zakas --- designs/2024-baseline-support/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index c5b7af38..955d7358 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -40,7 +40,9 @@ All paths are relative to CWD. ``` { "src/app/components/foobar/foobar.component.ts": { - "@typescript-eslint/no-explicit-any": 1 + "@typescript-eslint/no-explicit-any": { + count: 1 + } } } ``` From 98779dc8863079a1a1da2c33cc704e8da3ab7579 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Sat, 27 Jul 2024 10:13:17 +0300 Subject: [PATCH 08/27] Remove references to the deprecated engine --- designs/2024-baseline-support/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 955d7358..fa96bc9e 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -63,9 +63,9 @@ By default, the baseline file is saved at `.eslintbaseline` . To control where t eslint --baseline --baseline-location /home/user/project/mycache ``` -To implement this, we will need to add the two new options in `default-cli-options.js`, adjust the config for optionator and add the two new options as comments and arguments for both eslint and cli-engine. Documentation must be updated as well to explain the newly introduced options. +To implement this, we will need to add the two new options in `default-cli-options.js`, adjust the config for optionator and add the two new options as comments and arguments for both eslint. Documentation must be updated as well to explain the newly introduced options. -On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - the method must be called only and only if `--baseline` was provided and set to true. +On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` can be introduced in both `eslint.js` - the method must be called only and only if `--baseline` was provided and set to true. ### Matching against the baseline From b343ddb4084e4611dfcdf51a3e24624af5c10752 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Sat, 27 Jul 2024 10:27:05 +0300 Subject: [PATCH 09/27] Rename default baseline file to eslint-baseline.json --- designs/2024-baseline-support/README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index fa96bc9e..fc9843e9 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -49,7 +49,7 @@ All paths are relative to CWD. ### Generating the baseline -A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslintbaseline`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: +A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: ``` bash eslint --baseline ./src @@ -57,7 +57,7 @@ eslint --baseline ./src The above goes through each result item and messages, and counts the number of errors (`severity == 2`). If one or more such messages are found, the necessary details are stored in the baseline file. Note that the file is a relative path to CWD. -By default, the baseline file is saved at `.eslintbaseline` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. +By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. ``` bash eslint --baseline --baseline-location /home/user/project/mycache @@ -69,11 +69,11 @@ On top of that, we will need to adjust `cli.js` to check if `--baseline` was pro ### Matching against the baseline -The suggested solution always compares against the baseline, given that the baseline file exists. By default the baseline file is picked up from `.eslintbaseline`, unless the option `--baseline-location` is provided. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. As mentioned above, `--baseline-location` is a string argument specifying where the baseline was previously saved. +The suggested solution always compares against the baseline, given that the baseline file exists. By default the baseline file is picked up from `.eslint-baseline.json`, unless the option `--baseline-location` is provided. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. As mentioned above, `--baseline-location` is a string argument specifying where the baseline was previously saved. This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. -To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslintbaseline`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. A new method `applyBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - this must be called only and only if the baseline file exists. +To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslint-baseline.json`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. A new method `applyBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - this must be called only and only if the baseline file exists. We can also keep track of which errors from baseline were not matched, that is useful for the next section. @@ -83,9 +83,9 @@ When using the baseline, there is a chance that an error is resolved but the bas To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In `cli.js` we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose more we can display the list of errors that were left unmatched. -### Caching +### ESLint Cache -Caching must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: +ESLint cache (`--cache`) must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: - Generating the baseline can be based on the cache file and should be faster when the cache file is used. - Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. From ad5343de3f11689f5eaa97eea0bb866e9dcea2e8 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Sat, 27 Jul 2024 11:07:12 +0300 Subject: [PATCH 10/27] Include more implementation details --- designs/2024-baseline-support/README.md | 117 ++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 10 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index fc9843e9..09ffaf3d 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -33,9 +33,9 @@ This can be counterintuitive for enabling new rules as `error`, since the develo To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. -Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. +### Baseline format -All paths are relative to CWD. +The baseline files includes details about the file where the error found, the rule name and the number of errors. Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. All paths are relative to CWD, for portability reasons. ``` { @@ -49,23 +49,27 @@ All paths are relative to CWD. ### Generating the baseline -A new option `--baseline` can be added to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). For example: +A new option `--baseline` wil be introduced to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). Here is an example how to generate the baseline: ``` bash eslint --baseline ./src ``` -The above goes through each result item and messages, and counts the number of errors (`severity == 2`). If one or more such messages are found, the necessary details are stored in the baseline file. Note that the file is a relative path to CWD. - -By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option can be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. +The above goes through each result item and messages, and counts the number of errors (`severity == 2`). If one or more such messages are found, the necessary details are stored in the baseline file. By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option will be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. Here is an example +how to generate the baseline and save the results to `/home/user/project/mycache`. ``` bash eslint --baseline --baseline-location /home/user/project/mycache ``` -To implement this, we will need to add the two new options in `default-cli-options.js`, adjust the config for optionator and add the two new options as comments and arguments for both eslint. Documentation must be updated as well to explain the newly introduced options. +To introduce the above-mentioned options, we will need to: + + * add the new options in `default-cli-options.js`. + * adjust the config for optionator. + * add the new options as comments and arguments for eslint. + * update documentation to explain the newly introduced options. -On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` can be introduced in both `eslint.js` - the method must be called only and only if `--baseline` was provided and set to true. +On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` will be introduced, which must be called only and only if `--baseline` was provided and set to true. Please refer to "Implementation Details" for more details on this. ### Matching against the baseline @@ -73,7 +77,7 @@ The suggested solution always compares against the baseline, given that the base This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. -To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslint-baseline.json`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. A new method `applyBaseline` can be introduced in both `eslint.js` and `eslint-legacy.js` - this must be called only and only if the baseline file exists. +To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslint-baseline.json`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. We can also keep track of which errors from baseline were not matched, that is useful for the next section. @@ -81,7 +85,7 @@ We can also keep track of which errors from baseline were not matched, that is u When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. -To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In `cli.js` we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose more we can display the list of errors that were left unmatched. +To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In particular, we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose mode we can display the list of errors that were left unmatched. ### ESLint Cache @@ -91,6 +95,99 @@ ESLint cache (`--cache`) must contain the full list of detected errors, even tho - Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. - It even allows developers to delete the baseline and still take advantage of the cached file in subsequent runs. +### Implementation Details + +First of all we need to introduce a new type to hold the individual baseline result: + +``` js +/** + * @typedef {Record} BaselineResult + */ +``` + +One way to approach this is to introduce a Manager class handling the baseline results - in particular, both generating a baseline and validating the results against a baseline. + +``` js +class BaselineResultManager { + /** + * Creates a new instance of BaselineResultManager. + * @param {string} baselineLocation The location of the baseline file. + */ + constructor(baselineLocation) {} + + /** + * Generates the baseline from the provided lint results. + * @param {LintResult[]} results The lint results. + * @returns BaselineResult[] + */ + generateBaseline(results) + + /** + * Checks the provided baseline results against the lint results. + * It filters and returns the lint results that are not in the baseline. + * It also returns the unmatched baseline results. + * + * @param {LintResult[]} results The lint results. + * @param {BaselineResult[]} baseline The baseline. + * @return { + * results: LintResult[], + * unmatched: BaselineResult[] + * } + */ + applyBaseline(results, baseline) + + /** + * Loads the baseline from the file. + * @returns BaselineResult[] + */ + loadBaseline() + + /** + * Saves the baseline to the file. + * @param {BaselineResult[]} + * @returns void + * @private + */ + saveBaseline(baseline) +} +``` + +The resolution of the baseline location must happen outside of the above class. An idea is to make `getCacheFile` in `lib/eslint/eslint-helpers.js` a bit more abstract so that we can inject the prefix i.e. `.cache_` or `.baseline` when a directory is provided. This way both `cache-location` and `baseline-location` are consistent and following the same pattern. + +Once the above are in place, `cli.js` should look something like: + +``` js +// ... +const { baseline } = require("../conf/default-cli-options"); +// ... +if (options.quiet) { + debug("Quiet mode enabled - filtering out warnings"); + resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); +} + +const baselineFileLocation = getCacheFile(baseline, cwd, '_baseline'); +if (options.baseline || fs.existsSync(baseline)) { + const baselineManager = new BaselineResultManager(baselineFileLocation); + let loadedBaselineRecords = []; + if (options.baseline) { + loadedBaselineRecords = baselineManager.generateBaseline(resultsToPrint); + } else { + loadedBaselineRecords = baselineManager.loadBaseline(); + } + + const baselineResults = await baselineManager.applyBaseline(resultsToPrint, loadedBaselineRecords); + + if (baselineResults.unmatched.length > 0) { + // exit with errors + } + + resultsToPrint = baselineResults.results; +} + +const resultCounts = countErrors(results); +//... +``` + ## Documentation -Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example, is `no-explicit-any`. Unless the rule is enabled during the early stages of the project, it becomes harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. +Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example is [`@typescript-eslint/no-explicit-any`](https://typescript-eslint.io/rules/no-explicit-any/). Unless the rule is enabled during the early stages of the project, it becomes harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address future violations. From 70a7c569c0c98f5e1cba0ed5ab7814efeed57577 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Thu, 1 Aug 2024 21:08:25 +0300 Subject: [PATCH 12/27] Always update the baseline to update addressed violations Co-authored-by: Francesco Trotta --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index d162363e..840e2f45 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -55,7 +55,7 @@ A new option `--baseline` wil be introduced to ESLint CLI. When provided, the ba eslint --baseline ./src ``` -The above goes through each result item and messages, and counts the number of errors (`severity == 2`). If one or more such messages are found, the necessary details are stored in the baseline file. By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option will be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. Here is an example +The above goes through each result item and messages, and counts the number of errors (`severity == 2`). The necessary details are stored in the baseline file. By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option will be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. Here is an example how to generate the baseline and save the results to `/home/user/project/mycache`. ``` bash From 4462ce6d8475489123003c75946444b610dc2398 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Thu, 1 Aug 2024 21:12:25 +0300 Subject: [PATCH 13/27] Update designs/2024-baseline-support/README.md Co-authored-by: Francesco Trotta --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 840e2f45..6b504a85 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -165,7 +165,7 @@ if (options.quiet) { resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); } -const baselineFileLocation = getCacheFile(baseline, cwd, '_baseline'); +const baselineFileLocation = getCacheFile(baseline, cwd, 'baseline_'); if (options.baseline || fs.existsSync(baseline)) { const baselineManager = new BaselineResultManager(baselineFileLocation); let loadedBaselineRecords = []; From 1a1cbba8b392daa480c14c5d874e36fd24be02b5 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Thu, 1 Aug 2024 21:39:57 +0300 Subject: [PATCH 14/27] Add more details for matching against the baseline, and keeping the baseline lean --- designs/2024-baseline-support/README.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 6b504a85..a51b7323 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -77,7 +77,17 @@ The suggested solution always compares against the baseline, given that the base This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. -To implement this, we will need to adjust further `cli.js` and check if the baseline file exists - taking `--baseline-location` into consideration if exists, otherwise fallback to `.eslint-baseline.json`. This needs to take place right after the baseline is generated so that we take the baseline into consideration, if it was just generated. It also needs to take place before the error counting, so that the remaining errors are counted correctly. +To implement this, we will need to adjust further `cli.js` to adopt the following operations: + +* Check if the `--baseline` option is passed +> * If yes, we need to generate the baseline based on `resultsToPrint`. +> * If no, we need to check if the baseline file already exists taking `--baseline-location` into consideration +* Assuming that a baseline was found or generated, we need to match the errors found against the baseline. In particular, for each error found: +> * We need to check whether both file and error are part of the baseline +> * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. +> * If no, we keep the error. + +Note that `cli.js` the error detection in `cli.js` happens quite earlier before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". We can also keep track of which errors from baseline were not matched, that is useful for the next section. @@ -85,6 +95,13 @@ We can also keep track of which errors from baseline were not matched, that is u When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. +Consider the following scenario: + +* The developer executes `eslint --baseline ./src` which updates the baseline file. +* Then `eslint ./src` is executed which gives no violations, with an exit status of 0. +* The developer addresses an error violation. While the error is addressed is still part of the baseline. +* The developer then executes `eslint ./src` again. While it still gives no violations, it exits with a non-zero status code. That is to indicate that the baseline needs to be re-generated by executing `eslint --baseline ./src`. + To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In particular, we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose mode we can display the list of errors that were left unmatched. ### ESLint Cache From 71ee661d6adeb3db2cecbf7f724daa2b4479369f Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Thu, 1 Aug 2024 21:47:51 +0300 Subject: [PATCH 15/27] Fix lists formatting --- designs/2024-baseline-support/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index a51b7323..7b92e55d 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -80,12 +80,12 @@ This will go through each result item and message, and check each rule giving an To implement this, we will need to adjust further `cli.js` to adopt the following operations: * Check if the `--baseline` option is passed -> * If yes, we need to generate the baseline based on `resultsToPrint`. -> * If no, we need to check if the baseline file already exists taking `--baseline-location` into consideration + * If yes, we need to generate the baseline based on `resultsToPrint`. + * If no, we need to check if the baseline file already exists taking `--baseline-location` into consideration * Assuming that a baseline was found or generated, we need to match the errors found against the baseline. In particular, for each error found: -> * We need to check whether both file and error are part of the baseline -> * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. -> * If no, we keep the error. + * We need to check whether both file and error are part of the baseline + * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. + * If no, we keep the error. Note that `cli.js` the error detection in `cli.js` happens quite earlier before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". From 2d4dc84b27fc7849a1f2cfa052be32e5ac8acf3a Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Thu, 1 Aug 2024 21:56:29 +0300 Subject: [PATCH 16/27] Minor adjs --- designs/2024-baseline-support/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 7b92e55d..f5bec80c 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -64,10 +64,10 @@ eslint --baseline --baseline-location /home/user/project/mycache To introduce the above-mentioned options, we will need to: - * add the new options in `default-cli-options.js`. - * adjust the config for optionator. - * add the new options as comments and arguments for eslint. - * update documentation to explain the newly introduced options. +* add the new options in `default-cli-options.js`. +* adjust the config for optionator. +* add the new options as comments and arguments for eslint. +* update documentation to explain the newly introduced options. On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` will be introduced, which must be called only and only if `--baseline` was provided and set to true. Please refer to "Implementation Details" for more details on this. @@ -87,7 +87,7 @@ To implement this, we will need to adjust further `cli.js` to adopt the followin * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. * If no, we keep the error. -Note that `cli.js` the error detection in `cli.js` happens quite earlier before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". +Note that the error detection in `cli.js` happens before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". We can also keep track of which errors from baseline were not matched, that is useful for the next section. From dc2d94019072055ef243a652dc88242c7c4ff0f6 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Fri, 2 Aug 2024 22:59:48 +0300 Subject: [PATCH 17/27] First iteration to replace the concept of baseline with suppressions. --- designs/2024-baseline-support/README.md | 221 ++++++++++++++---------- 1 file changed, 126 insertions(+), 95 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index f5bec80c..1613d294 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -3,13 +3,13 @@ - RFC PR: (leave this empty, to be filled in later) - Authors: [Iacovos Constantinou](https://github.com/softius) -# Introduce baseline system to suppress existing errors +# Introduce a way to suppress violations ## Summary -Declare currently reported errors as the "baseline", so that they are not being reported in subsequent runs. It allows developers to enable one or more rules as `error` and be notified when new ones show up. +Suppress existing violations, so that they are not being reported in subsequent runs. It allows developers to enable one or more lint rules and be notified only when new violations show up. ## Motivation @@ -18,7 +18,7 @@ outcome? --> Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example is [`@typescript-eslint/no-explicit-any`](https://typescript-eslint.io/rules/no-explicit-any/). Unless the rule is enabled during the early stages of the project, it becomes harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. -This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address future violations. +This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address them. ## Detailed Design @@ -31,11 +31,11 @@ This can be counterintuitive for enabling new rules as `error`, since the develo used. Be sure to define any new terms in this section. --> -To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. +To keep track of the all the violations that we would like to suppress, we are storing these into a separate file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, no violations are suppressed - in other words, this feature doesn't affect existing or new projects, unless the developers explicitly suppress one or more violations. -### Baseline format +### File format -The baseline files includes details about the file where the error found, the rule name and the number of errors. Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. All paths are relative to CWD, for portability reasons. +The JSON file includes details about the file where the violations found, the rule name and the number of violations. As an example, the following indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one violation for the rule `@typescript-eslint/no-explicit-any` that we want to suppress. All paths are relative to CWD, for portability reasons. ``` { @@ -47,161 +47,191 @@ The baseline files includes details about the file where the error found, the ru } ``` -### Generating the baseline +The file is stored in `.eslint-suppressions.json` , unless otherwise specified. -A new option `--baseline` wil be introduced to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). Here is an example how to generate the baseline: +### Suppressing all violations + +A new option `--suppress-all` wil be introduced to ESLint CLI. When provided, the JSON file is generated and saved in `.eslint-suppressions.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). ``` bash -eslint --baseline ./src +eslint --suppress-all ./src ``` -The above goes through each result item and messages, and counts the number of errors (`severity == 2`). The necessary details are stored in the baseline file. By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option will be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. Here is an example -how to generate the baseline and save the results to `/home/user/project/mycache`. +### Suppressing violations of a specific rule + +A new option `--suppress-rule [RULE1]` will be introduced to ESLint CLI. When provided, the existing suppressions file will be updated to include any existing violation of the provided rule. The suppressions file will be created if not already exists. Note that this is a string flag option (value is required). ``` bash -eslint --baseline --baseline-location /home/user/project/mycache +eslint --suppress-rule '@typescript-eslint/no-explicit-any' ./src ``` -To introduce the above-mentioned options, we will need to: +### Changing the location of the suppressions file -* add the new options in `default-cli-options.js`. -* adjust the config for optionator. -* add the new options as comments and arguments for eslint. -* update documentation to explain the newly introduced options. +A new option `--suppression-location` will be introduced to ESLint CLI. When provided, the suppressions file will be loaded and saved to the provided location. Note that this is a string flag option (value is required). -On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` will be introduced, which must be called only and only if `--baseline` was provided and set to true. Please refer to "Implementation Details" for more details on this. +``` bash +eslint --suppress-all --suppressions-location /home/user/project/mycache ./src +``` -### Matching against the baseline +### Maintaining a lean suppressions file -The suggested solution always compares against the baseline, given that the baseline file exists. By default the baseline file is picked up from `.eslint-baseline.json`, unless the option `--baseline-location` is provided. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. As mentioned above, `--baseline-location` is a string argument specifying where the baseline was previously saved. +When working with suppressed violations, there is a chance that a violation is addressed but the suppressions file is not updated. This might allow new violations to creep in without noticing. To ensure that the suppressions file is always up to date, `eslint` can exit with an error code when there are suppressed violations that do not occur anymore. -This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. +Consider the following scenario: -To implement this, we will need to adjust further `cli.js` to adopt the following operations: +* The developer executes `eslint --supress-all ./src` which creates the suppressions file. +* Then `eslint ./src` is executed which reports no violations, with an exit status of 0. +* The developer addresses an error violation. While the violation is addressed is still part of the suppressions file. +* The developer then executes `eslint ./src` again. While it still reports no violations, it exits with a non-zero status code. That is to indicate that the suppressions file needs to be updated. -* Check if the `--baseline` option is passed - * If yes, we need to generate the baseline based on `resultsToPrint`. - * If no, we need to check if the baseline file already exists taking `--baseline-location` into consideration -* Assuming that a baseline was found or generated, we need to match the errors found against the baseline. In particular, for each error found: - * We need to check whether both file and error are part of the baseline - * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. - * If no, we keep the error. +To address this, a new option `--prune-suggestions` will be introduced to ESLint. Note that this is a boolean flag option (no values are accepted). When provided, violations in suppressions file that no longer occur will be removed, but no new violations will be added (in contrary to when executing `--suppress-all`). -Note that the error detection in `cli.js` happens before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". +``` bash +eslint --prune-suppressions ./src +eslint --prune-suppressions --suppressions-location /home/user/project/mycache ./src +``` -We can also keep track of which errors from baseline were not matched, that is useful for the next section. +### Execution details -### Maintaining a lean baseline +The suggested solution always compares against the suppressions file, given that the file already exists. By default the file is picked up from `.eslint-suppressions.json`, unless the option `--suppressions-location` is provided. This makes it easier for existing and new projects to adopt this feature without the need to adjust scripts in `package.json` and CI/CD workflows. -When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. +This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the suppressions. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the suppressions file, it means that we can remove and ignore the result message. -Consider the following scenario: +To implement this, we will need to adjust mainly `cli.js` to adopt the following operations: -* The developer executes `eslint --baseline ./src` which updates the baseline file. -* Then `eslint ./src` is executed which gives no violations, with an exit status of 0. -* The developer addresses an error violation. While the error is addressed is still part of the baseline. -* The developer then executes `eslint ./src` again. While it still gives no violations, it exits with a non-zero status code. That is to indicate that the baseline needs to be re-generated by executing `eslint --baseline ./src`. +* Check if the `--suppress-all` or `--suppress-rule` option is passed + * If both are passed exit with an error, since the two options are mutually exclusive. + * If either option was passed, we need to update the suppressions file based on `results`. + * If none option was passed, we need to check if the suppressions file already exists taking `--suppressions-location` into consideration +* Assuming that a suppressions file was found or generated, we need to match the errors found against the violations included in the suppressions file. In particular, for each error found: + * We need to check whether both file and error are part of the suppressions file. + * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. + * If no, we keep the error. +* Exit with a non-zero status code if there are unmatched violations. Depending on the verbose mode we can display the list of errors that were left unmatched. +* Otherwise list the remaining errors as usual. -To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In particular, we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose mode we can display the list of errors that were left unmatched. +Note that the error detection in `cli.js` happens before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. -### ESLint Cache +Furthermore, ESLint cache (`--cache`) must contain the full list of detected violations, even those matched against the suppressions file. This approach has the following benefits: -ESLint cache (`--cache`) must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: +- Generating the suppressions file can be based on the cache file and should be faster when the cache file is used. +- Allows developers to re-generate the suppressions file or even adjust it manually and re-lint still taking the same cache into consideration. +- It even allows developers to delete the suppressions file and still take advantage of the cached file in subsequent runs. -- Generating the baseline can be based on the cache file and should be faster when the cache file is used. -- Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. -- It even allows developers to delete the baseline and still take advantage of the cached file in subsequent runs. +# Implementation notes -### Implementation Details +To introduce the above-mentioned options, we will need to: -First of all we need to introduce a new type to hold the individual baseline result: +* add the new options in `default-cli-options.js`. +* adjust the config for optionator. +* add the new options as comments and arguments for eslint. +* update documentation to explain the newly introduced option + +A new type must be created: ``` js /** - * @typedef {Record} BaselineResult + * @typedef {Record} SuppressedViolation */ ``` -One way to approach this is to introduce a Manager class handling the baseline results - in particular, both generating a baseline and validating the results against a baseline. - ``` js -class BaselineResultManager { +class SuppressedViolationsManager { /** - * Creates a new instance of BaselineResultManager. - * @param {string} baselineLocation The location of the baseline file. + * Creates a new instance of SuppressedViolationsManager. + * @param {string} suppressionsLocation The location of the suppressions file. */ - constructor(baselineLocation) {} + constructor(suppressionsLocation) {} /** - * Generates the baseline from the provided lint results. + * Updates the suppressions file based on the current violations + * * @param {LintResult[]} results The lint results. - * @returns BaselineResult[] + * @returns SuppressedViolation[] */ - generateBaseline(results) + suppressAll(results) /** - * Checks the provided baseline results against the lint results. - * It filters and returns the lint results that are not in the baseline. - * It also returns the unmatched baseline results. + * Updates the suppressions file based on the current violations and the provided rule * * @param {LintResult[]} results The lint results. - * @param {BaselineResult[]} baseline The baseline. + * @param {string} rule The rule to suppress. + */ + suppressByRule(results, rule) + + /** + * Removes old suppressions that do not occur anymore. + * @returns void + */ + prune() + + /** + * Checks the provided suppressions against the lint results. + * It filters and returns the lint results that are not in the suppressions file. + * It also returns the unmatched suppressions. + * + * @param {LintResult[]} results The lint results. + * @param {SuppressedViolation[]} suppressions The suppressions. * @return { * results: LintResult[], - * unmatched: BaselineResult[] + * unmatched: SuppressedViolation[] * } */ - applyBaseline(results, baseline) + applySuppressions(results, suppressions) /** - * Loads the baseline from the file. - * @returns BaselineResult[] + * Loads the suppressions file. + * @returns SuppressedViolation[] */ - loadBaseline() + load() /** - * Saves the baseline to the file. - * @param {BaselineResult[]} + * Updates the suppressions file. + * @param {SuppressedViolation[]} * @returns void * @private */ - saveBaseline(baseline) + save() } ``` -The resolution of the baseline location must happen outside of the above class. An idea is to make `getCacheFile` in `lib/eslint/eslint-helpers.js` a bit more abstract so that we can inject the prefix i.e. `.cache_` or `.baseline` when a directory is provided. This way both `cache-location` and `baseline-location` are consistent and following the same pattern. +The resolution of the suppressions file must happen outside of the above class. An idea is to make `getCacheFile` in `lib/eslint/eslint-helpers.js` a bit more abstract so that we can inject the prefix i.e. `.cache_` or `.suppressions_` when a directory is provided. This way both `cache-location` and `suppressions-location` are consistent and following the same pattern. Once the above are in place, `cli.js` should look something like: ``` js // ... -const { baseline } = require("../conf/default-cli-options"); -// ... -if (options.quiet) { - debug("Quiet mode enabled - filtering out warnings"); - resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); +if (options.fix) { + debug("Fix mode enabled - applying fixes"); + await ActiveESLint.outputFixes(results); } -const baselineFileLocation = getCacheFile(baseline, cwd, 'baseline_'); -if (options.baseline || fs.existsSync(baseline)) { - const baselineManager = new BaselineResultManager(baselineFileLocation); - let loadedBaselineRecords = []; - if (options.baseline) { - loadedBaselineRecords = baselineManager.generateBaseline(resultsToPrint); +const suppressionsFileLocation = getCacheFile(options.suppressionsLocation, cwd, 'suppressions_'); +if (options.suppressAll || options.suppressRule || options.pruneSuppressions || fs.existsSync(suppressionsFileLocation)) { + const suppressionsManager = new SuppressedViolationsManager(suppressionsFileLocation); + if (options.suppressAll) { + results = suppressionsManager.suppressAll(results); + } else if (options.suppressRule) { + results = suppressionsManager.suppressByRule(results, options.suppressRule); + } else if (options.pruneSuppressions) { + results = suppressionsManager.prune(); } else { - loadedBaselineRecords = baselineManager.loadBaseline(); - } - - const baselineResults = await baselineManager.applyBaseline(resultsToPrint, loadedBaselineRecords); + const suppressionResults = suppressionsManager.applySuppressions(results, suppressionsManager.load()); + if (suppressionResults.unmatched.length > 0) { + // exit with a non-zero code + } - if (baselineResults.unmatched.length > 0) { - // exit with errors + results = suppressionResults.results; } +} + +let resultsToPrint = results; - resultsToPrint = baselineResults.results; +if (options.quiet) { + debug("Quiet mode enabled - filtering out warnings"); + resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); } -const resultCounts = countErrors(results); //... ``` @@ -212,7 +242,7 @@ const resultCounts = countErrors(results); on the ESLint blog to explain the motivation? --> -We should update [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface) to document the newly introduced option. A dedicated section should be added in Documentation to explain how baseline works. +We should update [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface) to document the newly introduced options. A dedicated section should be added in Documentation to explain how the new suppression system works. ## Drawbacks @@ -227,7 +257,7 @@ We should update [Command Line Interface Reference](https://eslint.org/docs/late implementing this RFC as possible. --> -The baseline can be generated and used only when linting files. It can not be leveraged when using `stdin` since it relies on file paths. +The suggested solution can be used only when linting files. It can not be leveraged when using `stdin` since it relies on file paths. ## Backwards Compatibility Analysis @@ -237,11 +267,11 @@ The baseline can be generated and used only when linting files. It can not be le to existing users? --> -If the baseline file is not generated, ESLint CLI behavior will not change. This change is therefore backwards compatible to start with. +If the suppressions file does not exist, ESLint CLI behavior will not change. This change is therefore backwards compatible to start with. -If the baseline file is generated, ESLint CLI will compare the errors against the errors included in the baseline. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the baseline without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the baseline can be easily deleted and cancel the new behavior. +If the suppressions file is already generated, ESLint CLI will compare the errors against the violations included in the suppressions file. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the suggested solution without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the suppressions file can be easily deleted and cancel the new behavior. -Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean baseline"). This might have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. +Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean suppressions file"). This might have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. ## Alternatives @@ -252,7 +282,7 @@ Furthermore, we are adding one more reason to exit with an error code (see "Main projects have already implemented a similar feature. --> -Unfortunately existing approaches do not address the issue at its core and come with their own set of drawbacks. It is worth mentioning that the suggested solution is based on [how baseline works in PHPStan](https://phpstan.org/user-guide/baseline). +Unfortunately existing approaches do not address the issue at its core and come with their own set of drawbacks. It is worth mentioning that the suggested solution is based on [how baseline works in PHPStan](https://phpstan.org/user-guide/baseline) and bulk suppressions from [@rushstack/eslint-bulk](https://www.npmjs.com/package/@rushstack/eslint-bulk). The following sections are extracted from [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) where [@jfmengels](https://github.com/jfmengels) did a detailed analysis about existing approaches and their drawbacks. @@ -321,12 +351,13 @@ I expect to implement this change. ### Does this count warnings? -No, we are only counting errors when generating the baseline. Also only errors are considered when checking against the baseline. +No, we are only counting errors when updating the suppressions file. Also only errors are considered when checking against the suppressions file. ## Related Discussions * [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) * [PHPStan - The Baseline](https://phpstan.org/user-guide/baseline) +* [@rushstack/eslint-bulk](https://www.npmjs.com/package/@rushstack/eslint-bulk) -To keep track of the all the violations that we would like to suppress, we are storing these into a separate file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, no violations are suppressed - in other words, this feature doesn't affect existing or new projects, unless the developers explicitly suppress one or more violations. +We are storing all the violations that we would like to suppress into a separate file. This file is a JSON file containing the number of errors that must be ignored for each rule in each file. By design, no violations are suppressed - in other words, this feature doesn't affect existing or new projects, unless the developers explicitly suppress one or more violations. ### File format -The JSON file includes details about the file where the violations found, the rule name and the number of violations. As an example, the following indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one violation for the rule `@typescript-eslint/no-explicit-any` that we want to suppress. All paths are relative to CWD, for portability reasons. +The JSON file includes details about the file where the violations are found, the rule name and the number of violations. As an example, the following indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one violation for the rule `@typescript-eslint/no-explicit-any` that we want to suppress. All paths are relative to CWD, for portability reasons. ``` { @@ -75,16 +75,16 @@ eslint --suppress-all --suppressions-location /home/user/project/mycache ./src ### Maintaining a lean suppressions file -When working with suppressed violations, there is a chance that a violation is addressed but the suppressions file is not updated. This might allow new violations to creep in without noticing. To ensure that the suppressions file is always up to date, `eslint` can exit with an error code when there are suppressed violations that do not occur anymore. +When working with suppressed violations, it's possible to address a violation without updating the suppressions file. This oversight can allow new violations to go unnoticed. To prevent this, eslint can exit with an error code if there are outdated (unmatched) suppressions. Consider the following scenario: -* The developer executes `eslint --supress-all ./src` which creates the suppressions file. -* Then `eslint ./src` is executed which reports no violations, with an exit status of 0. -* The developer addresses an error violation. While the violation is addressed is still part of the suppressions file. -* The developer then executes `eslint ./src` again. While it still reports no violations, it exits with a non-zero status code. That is to indicate that the suppressions file needs to be updated. +* The developer runs `eslint --supress-all ./src` to create the suppressions file. +* Running `eslint ./src` reports no violations and exits with status 0. +* After fixing a violation, the suppressions file still contains the now-resolved violation. +* Running `eslint ./src` again reports no violations but exits with a non-zero status code, indicating the suppressions file needs updating. -To address this, a new option `--prune-suggestions` will be introduced to ESLint. Note that this is a boolean flag option (no values are accepted). When provided, violations in suppressions file that no longer occur will be removed, but no new violations will be added (in contrary to when executing `--suppress-all`). +To address this, a new option `--prune-suggestions` will be introduced to ESLint. This boolean flag removes resolved violations from the suppressions file without adding new ones, unlike `--suppress-all`. ``` bash eslint --prune-suppressions ./src @@ -93,29 +93,34 @@ eslint --prune-suppressions --suppressions-location /home/user/project/mycache . ### Execution details -The suggested solution always compares against the suppressions file, given that the file already exists. By default the file is picked up from `.eslint-suppressions.json`, unless the option `--suppressions-location` is provided. This makes it easier for existing and new projects to adopt this feature without the need to adjust scripts in `package.json` and CI/CD workflows. +The suggested solution always compares against the existing suppressions file, typically `.eslint-suppressions.json`, unless `--suppressions-location` is specified. This makes it easier for existing and new projects to adopt this feature without the need to adjust scripts in `package.json` and CI/CD workflows. -This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the suppressions. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the suppressions file, it means that we can remove and ignore the result message. +To perform the comparison, we will go through each result and message from `ESLint.lintFiles`, checking each error `(severity == 2)` against the suppressions file. By design, we ignore warnings since they don't cause eslint to exit with an error code and serve a different purpose. If the file and rule are listed in the suppressions file, we can remove and ignore the result message. -To implement this, we will need to adjust mainly `cli.js` to adopt the following operations: +Here is a high-level overview of the execution flow: +1. **Check for Options** + * If both `--suppress-all` and `--suppress-rule` are passed, exit with an error (these options are mutually exclusive). + * If either option is passed, update the suppressions file based on the `results`. + * If no option is passed, check if the suppressions file exists, considering `--suppressions-location`. +2. **Match Errors Against Suppressions** + * For each error, check if it and the file are in the suppressions file. + * If yes, decrease count by 1 and ignore the error unless count is zero. + * If no, keep the error. +3. Report and exit + * Exit with a non-zero status if there are unmatched violations, optionally listing them in verbose mode. + * Otherwise, list remaining errors as usual. * Check if the `--suppress-all` or `--suppress-rule` option is passed * If both are passed exit with an error, since the two options are mutually exclusive. * If either option was passed, we need to update the suppressions file based on `results`. * If none option was passed, we need to check if the suppressions file already exists taking `--suppressions-location` into consideration -* Assuming that a suppressions file was found or generated, we need to match the errors found against the violations included in the suppressions file. In particular, for each error found: - * We need to check whether both file and error are part of the suppressions file. - * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. - * If no, we keep the error. -* Exit with a non-zero status code if there are unmatched violations. Depending on the verbose mode we can display the list of errors that were left unmatched. -* Otherwise list the remaining errors as usual. -Note that the error detection in `cli.js` happens before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. +Note that the error detection in `cli.js` occurs before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. -Furthermore, ESLint cache (`--cache`) must contain the full list of detected violations, even those matched against the suppressions file. This approach has the following benefits: +Furthermore, ESLint cache (`--cache`) must include the full list of detected violations, even those in the suppressions file. This approach has the following benefits: - Generating the suppressions file can be based on the cache file and should be faster when the cache file is used. -- Allows developers to re-generate the suppressions file or even adjust it manually and re-lint still taking the same cache into consideration. +- Allows developers to update the suppressions file and then re-lint still taking the same cache into consideration. - It even allows developers to delete the suppressions file and still take advantage of the cached file in subsequent runs. ### Implementation notes @@ -125,7 +130,7 @@ To introduce the above-mentioned options, we will need to: * add the new options in `default-cli-options.js`. * adjust the config for optionator. * add the new options as comments and arguments for eslint. -* update documentation to explain the newly introduced option +* update documentation to explain the newly introduced feature. A new type must be created: From 4fea00ea16be92239f749d551f7eefb11ef5bb4a Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Sat, 3 Aug 2024 07:32:58 +0300 Subject: [PATCH 20/27] Revise return types --- designs/2024-baseline-support/README.md | 27 +++++++++++++------------ 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 7b51fd24..3d2152e1 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -132,11 +132,11 @@ To introduce the above-mentioned options, we will need to: * add the new options as comments and arguments for eslint. * update documentation to explain the newly introduced feature. -A new type must be created: +A new type must be created to represent the suppressions file: ``` js /** - * @typedef {Record} SuppressedViolation + * @typedef {Record} SuppressedViolations */ ``` @@ -154,21 +154,22 @@ class SuppressedViolationsManager { * Updates the suppressions file based on the current violations. * * @param {LintResult[]} results The lint results. - * @returns {LintResult[]} + * @returns {void} */ suppressAll(results) /** - * Updates the suppressions file based on the current violations and the provided rule + * Updates the suppressions file based on the current violations and the provided rule. * * @param {LintResult[]} results The lint results. * @param {string} rule The rule to suppress. + * @returns {void} */ suppressByRule(results, rule) /** * Removes old suppressions that do not occur anymore. - * @returns void + * @returns {void} */ prune() @@ -178,27 +179,27 @@ class SuppressedViolationsManager { * It also returns the unmatched suppressions. * * @param {LintResult[]} results The lint results. - * @param {SuppressedViolation[]} suppressions The suppressions. - * @return { + * @param {SuppressedViolations} suppressions The suppressions. + * @returns {{ * results: LintResult[], - * unmatched: SuppressedViolation[] - * } + * unmatched: SuppressedViolations + * }} */ applySuppressions(results, suppressions) /** * Loads the suppressions file. - * @returns SuppressedViolation[] + * @returns {SuppressedViolations} */ load() /** * Updates the suppressions file. - * @param {SuppressedViolation[]} - * @returns void + * @param {SuppressedViolations} suppressions The suppressions to save. + * @returns {void} * @private */ - save() + save(suppressions) } ``` From 42d8b95e19f201776a85e47ab62cc3e1fc3aa8b3 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Sat, 3 Aug 2024 07:38:49 +0300 Subject: [PATCH 21/27] Minor cleanup --- designs/2024-baseline-support/README.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 3d2152e1..0fc58d50 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -107,13 +107,9 @@ Here is a high-level overview of the execution flow: * For each error, check if it and the file are in the suppressions file. * If yes, decrease count by 1 and ignore the error unless count is zero. * If no, keep the error. -3. Report and exit +3. **Report and exit** * Exit with a non-zero status if there are unmatched violations, optionally listing them in verbose mode. * Otherwise, list remaining errors as usual. -* Check if the `--suppress-all` or `--suppress-rule` option is passed - * If both are passed exit with an error, since the two options are mutually exclusive. - * If either option was passed, we need to update the suppressions file based on `results`. - * If none option was passed, we need to check if the suppressions file already exists taking `--suppressions-location` into consideration Note that the error detection in `cli.js` occurs before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. @@ -224,7 +220,7 @@ if (options.suppressAll || options.suppressRule || options.pruneSuppressions || } else if (options.pruneSuppressions) { suppressionsManager.prune(); } - + const suppressionResults = suppressionsManager.applySuppressions(results, suppressionsManager.load()); if (suppressionResults.unmatched.length > 0) { // exit with a non-zero code From bff622da49c90f51f6c7692319f12045758779f5 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Tue, 6 Aug 2024 22:14:39 +0300 Subject: [PATCH 22/27] Allow to pass multiple rules --- designs/2024-baseline-support/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 0fc58d50..e1e0ba79 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -59,10 +59,10 @@ eslint --suppress-all ./src ### Suppressing violations of a specific rule -A new option `--suppress-rule [RULE1]` will be introduced to ESLint CLI. When provided, the existing suppressions file will be updated to include any existing violation of the provided rule. The suppressions file will be created if not already exists. Note that this is a string flag option (value is required). +A new option `--suppress-rule [RULE1]` will be introduced to ESLint CLI. When provided, the existing suppressions file will be updated to include any existing violation of the provided rule. The suppressions file will be created if not already exists. Note that this is option can accept an array of string values. ``` bash -eslint --suppress-rule '@typescript-eslint/no-explicit-any' ./src +eslint --suppress-rule '@typescript-eslint/no-explicit-any' --suppress-rul '@typescript-eslint/member-ordering' ./src ``` ### Changing the location of the suppressions file @@ -158,10 +158,10 @@ class SuppressedViolationsManager { * Updates the suppressions file based on the current violations and the provided rule. * * @param {LintResult[]} results The lint results. - * @param {string} rule The rule to suppress. + * @param {string[]} rules The rules to suppress. * @returns {void} */ - suppressByRule(results, rule) + suppressByRule(results, rules) /** * Removes old suppressions that do not occur anymore. From 48e9d0a41369abfbd8275569e2a0679e6b1c0eba Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Tue, 6 Aug 2024 22:16:30 +0300 Subject: [PATCH 23/27] Fix typo --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index e1e0ba79..2ca004db 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -84,7 +84,7 @@ Consider the following scenario: * After fixing a violation, the suppressions file still contains the now-resolved violation. * Running `eslint ./src` again reports no violations but exits with a non-zero status code, indicating the suppressions file needs updating. -To address this, a new option `--prune-suggestions` will be introduced to ESLint. This boolean flag removes resolved violations from the suppressions file without adding new ones, unlike `--suppress-all`. +To address this, a new option `--prune-suppressions` will be introduced to ESLint. This boolean flag removes resolved violations from the suppressions file without adding new ones, unlike `--suppress-all`. ``` bash eslint --prune-suppressions ./src From 46cf6ae49e76b688dd56bcaa08c04df12c2b7984 Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Fri, 9 Aug 2024 07:23:26 +0300 Subject: [PATCH 24/27] Update designs/2024-baseline-support/README.md Co-authored-by: Nicholas C. Zakas --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 2ca004db..be425d2e 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -161,7 +161,7 @@ class SuppressedViolationsManager { * @param {string[]} rules The rules to suppress. * @returns {void} */ - suppressByRule(results, rules) + suppressRules(results, rules) /** * Removes old suppressions that do not occur anymore. From a94a50d6e654410840d6227c7dde8924e600b8ca Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Fri, 16 Aug 2024 17:01:34 +0300 Subject: [PATCH 25/27] Fix typo Co-authored-by: Francesco Trotta --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index be425d2e..4a83c4ce 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -67,7 +67,7 @@ eslint --suppress-rule '@typescript-eslint/no-explicit-any' --suppress-rul '@typ ### Changing the location of the suppressions file -A new option `--suppression-location` will be introduced to ESLint CLI. When provided, the suppressions file will be loaded and saved to the provided location. Note that this is a string flag option (value is required). +A new option `--suppressions-location` will be introduced to ESLint CLI. When provided, the suppressions file will be loaded and saved to the provided location. Note that this is a string flag option (value is required). ``` bash eslint --suppress-all --suppressions-location /home/user/project/mycache ./src From 861f1a44489656e3524752d3a39a4550dc43a30f Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou <586121+softius@users.noreply.github.com> Date: Fri, 16 Aug 2024 17:03:25 +0300 Subject: [PATCH 26/27] Use block comments Co-authored-by: Francesco Trotta --- designs/2024-baseline-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index 4a83c4ce..bf34b826 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -300,7 +300,7 @@ Also, users can too easily ignore the new errors, so in a way, the rule is enabl ### Using disable comments -One can use disable comments to temporarily suppress errors, by adding a comment like `// eslint-disable rule-name -- FIX THIS LATER` +One can use disable comments to temporarily suppress errors, by adding a comment like `/* eslint-disable rule-name -- FIX THIS LATER */` "Disable comments" can be used to enable a rule as an error early, by adding them everywhere where an error is currently reported (and that is actually something that can be automated by some linters). From 566e3b951c15515feb341cb24f0af4345109486f Mon Sep 17 00:00:00 2001 From: Iacovos Constantinou Date: Fri, 16 Aug 2024 17:21:09 +0300 Subject: [PATCH 27/27] More details about prune-suggestions and how the filtering works. --- designs/2024-baseline-support/README.md | 30 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index bf34b826..1cca6609 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -100,16 +100,19 @@ To perform the comparison, we will go through each result and message from `ESLi Here is a high-level overview of the execution flow: 1. **Check for Options** - * If both `--suppress-all` and `--suppress-rule` are passed, exit with an error (these options are mutually exclusive). - * If either option is passed, update the suppressions file based on the `results`. - * If no option is passed, check if the suppressions file exists, considering `--suppressions-location`. + * If both `--suppress-all` and `--suppress-rule` are passed, exit with an error (these options are mutually exclusive). + * If either option is passed, update the suppressions file based on the `results`. + * If no option is passed, check if the suppressions file exists, considering `--suppressions-location`. 2. **Match Errors Against Suppressions** - * For each error, check if it and the file are in the suppressions file. - * If yes, decrease count by 1 and ignore the error unless count is zero. - * If no, keep the error. -3. **Report and exit** - * Exit with a non-zero status if there are unmatched violations, optionally listing them in verbose mode. - * Otherwise, list remaining errors as usual. + * For each error, check if the error and the file are in the suppressions file. + * If yes, decrease count, in memory, by 1 and ignore the error unless count is zero. + * If no, keep the error. +3. **Prune unmatched suppressions** + * If `--prune-suppressions` is passed, take the updated suppressions from memory to check which suppressions are left. + * For each suppression left, update the suppressions file by either reducing the count or removing the suppression. +4. **Report and exit** + * Exit with a non-zero status if there are unmatched violations, optionally listing them in verbose mode. + * Otherwise, list remaining errors as usual. Note that the error detection in `cli.js` occurs before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. @@ -171,8 +174,13 @@ class SuppressedViolationsManager { /** * Checks the provided suppressions against the lint results. - * It filters and returns the lint results that are not in the suppressions file. - * It also returns the unmatched suppressions. + * + * For each error included in `results`, checks if the error and the file are in `suppressions`. + * If yes, the count is decreased by 1 and ignores the error unless the count has reached zero. + * Otherwise, it keeps the error. + * + * It returns the lint results that are not in the suppressions file, + * as well as the unmatched suppressions. * * @param {LintResult[]} results The lint results. * @param {SuppressedViolations} suppressions The suppressions.