Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Introduce a way to suppress violations #119

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a99c456
Add RFC for baseline support
softius Apr 22, 2024
f9b3976
Fix typos
softius Apr 22, 2024
c9a718c
Add question about warnings
softius May 6, 2024
4d73750
Convert relative to full paths
softius May 6, 2024
9e035f1
Replace --generate-baseline with --baseline/-location
softius May 6, 2024
ed01be1
Add more implementation details, add relative paths to CWD
softius May 8, 2024
d5411af
Update designs/2024-baseline-support/README.md
softius May 30, 2024
98779dc
Remove references to the deprecated engine
softius Jul 27, 2024
b343ddb
Rename default baseline file to eslint-baseline.json
softius Jul 27, 2024
ad5343d
Include more implementation details
softius Jul 27, 2024
485f684
Add link for no-explicit-any
softius Aug 1, 2024
70a7c56
Always update the baseline to update addressed violations
softius Aug 1, 2024
4462ce6
Update designs/2024-baseline-support/README.md
softius Aug 1, 2024
1a1cbba
Add more details for matching against the baseline, and keeping the b…
softius Aug 1, 2024
71ee661
Fix lists formatting
softius Aug 1, 2024
2d4dc84
Minor adjs
softius Aug 1, 2024
dc2d940
First iteration to replace the concept of baseline with suppressions.
softius Aug 2, 2024
b8a1cf7
Fix header and other minor adjustments
softius Aug 2, 2024
0c3d9a4
Simplify language
softius Aug 3, 2024
4fea00e
Revise return types
softius Aug 3, 2024
42d8b95
Minor cleanup
softius Aug 3, 2024
bff622d
Allow to pass multiple rules
softius Aug 6, 2024
48e9d0a
Fix typo
softius Aug 6, 2024
46cf6ae
Update designs/2024-baseline-support/README.md
softius Aug 9, 2024
a94a50d
Fix typo
softius Aug 16, 2024
861f1a4
Use block comments
softius Aug 16, 2024
566e3b9
More details about prune-suggestions and how the filtering works.
softius Aug 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Simplify language
  • Loading branch information
softius committed Aug 3, 2024
commit 0c3d9a48d08bb480d1c9f7a25e0ae47fc0e37eb1
47 changes: 26 additions & 21 deletions designs/2024-baseline-support/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.

```
{
Expand Down Expand Up @@ -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.
Copy link
Member

@mdjermanovic mdjermanovic Aug 29, 2024

Choose a reason for hiding this comment

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

In this case, will there be an error message, and what would it look like? Technically, will it be a lint message passed to the formatter along with other lint messages for the file, or a separate output?


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
Expand All @@ -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).
softius marked this conversation as resolved.
Show resolved Hide resolved
* 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
Expand All @@ -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:

Expand Down