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
24 changes: 17 additions & 7 deletions designs/2024-baseline-support/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,44 +33,54 @@ 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.
softius marked this conversation as resolved.
Show resolved Hide resolved

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.
softius marked this conversation as resolved.
Show resolved Hide resolved

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
softius marked this conversation as resolved.
Show resolved Hide resolved
}
}
```

### 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:
softius marked this conversation as resolved.
Show resolved Hide resolved

``` 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.
softius marked this conversation as resolved.
Show resolved Hide resolved

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.
softius marked this conversation as resolved.
Show resolved Hide resolved

### 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.

### 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.
Copy link
Member

Choose a reason for hiding this comment

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

By resolved you mean reported? If an error that is not filtered out by the baseline is reported, I assume that ESLint will display the error message and exit with an error code as usual. It is not clear to me why that would go unnoticed.

Copy link
Author

Choose a reason for hiding this comment

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

@fasttime By resolved I am referring to a newly addressed violation. I have added a scenario as an example.

The purpose of this section, is to encourage developers re-generating the baseline after addressing one or more violations.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks. Actually we don't know which errors are resolved. We can only compare the number of errors before and after the current run. If an edit fixes some errors for a rule but introduces some errors for another rule, re-generating the baseline would cause the newly introduced errors to be ignored in subsequent runs. I'm not sure that makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. This assumes there is an additional control of the baseline, through PRs as an example.

There is a suggestion to change the approach to be more close to what eslint-bulk provides. In that case, a new option will be introduced called --prune-suprressions as briefly described here which I believe covers this gap.


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:
softius marked this conversation as resolved.
Show resolved Hide resolved
Expand Down