feat: Introduce baseline system to suppress existing errors #119
Conversation
| 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. |
There was a problem hiding this comment.
the issue with using a count is that it's possible to change the file without changing the count - i.e. you add as many new reports as you remove.
EG in this example the code has meaningfully changed, but the total number of reports is unchanged. So the file would not violate the baseline.
/// before
/* eslint no-console: error */
export function test(condition) {
if (condition) {
console.log();
} else {
return 1;
}
}
/// after
/* eslint no-console: error */
export function test(condition) {
if (condition) {
return 1;
} else {
console.log();
}
}personally I've preferred more granular checks - eg the line + col as it's stricter. It's more likely to false-positive, but it's less likely to false-negative (which is more valuable, IMO).
There was a problem hiding this comment.
Hey, PHPStan creator here (where the baseline is implemented for the past almost 5 years). It's the most popular feature by far (yeah, the most popular feature of a static analyser is ignoring errors).
Turns out that the count is the right amount of granularity. Expecting "having this error in this file this many times is okay" is really useful. If you included the line number in the baseline entry, it'd get obsolete really fast. It'd essentially be an inline ignore, but in an external file. Not really practical.
If someone manages to remove an error but add it in a different place in the same file, good for them. Having a baseline means you're ignoring some errors on purpose, and this situation most likely means it's the same error, it just moved.
PHPStan is a little bit more granular becauses the baseline includes exact error messages being ignored, but this proposal only includes error types. Maybe that's something to consider.
An alternative approach that preserves line numbers would be to also include the git commit hash from the moment the baseline was generated. So "this error on line 12 in commit 34afddac". This would allow shifting expected errors around based on git diff. So three commits later, if there was a new line added on line 5, the baseline would expect the error to occur on line 13.
But that can become computationally expensive as the diff between the original commit and HEAD grows bigger (if the baseline is not regenerated for a long time).
Anyway, I'm really excited other programming tools to adopt the baseline concept, it's really useful!
There was a problem hiding this comment.
From my point view, the baseline is an "agreement" between the developers and eslint that it is acceptable to have X number of errors, from a particular rule, for a particular file.
You are both right @ondrejmirtes @bradzacher , someone could fix one error and create another one or move it to a different line and this won't be reported. Personally, I don't see that as a problem. If that is necessary, maybe it can be resolved at the git / ci/cd level similarly to the suggestion provided by @bradzacher below.
There was a problem hiding this comment.
I think the error count is fine for a first pass at this feature.
|
|
||
| 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 |
There was a problem hiding this comment.
the alternative we've introduced at Canva is that we designate specific rules as "in migration" and we only consider reports from those rules if they exist in changed files (according to git comparison against the main branch).
With this system developers must address lint errors if they touch a file but otherwise they can be ignored.
This does require integration with the relevant source control system - though we've found it works quite well.
|
Thanks for the RFC @softius. Can you please sign the CLA as requested in this comment? |
|
|
||
| ### 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: |
There was a problem hiding this comment.
In case of PHPStan, this flag optionally accepts a filename. In some cases people generate multiple different baselines. It allows you to have a different baseline from the points in time where you enabled different rules.
It'd also mean that these baseline files would have to be referenced from the main eslint config because they couldn't be auto-discovered anymore. That's a potential downside.
There was a problem hiding this comment.
Yes, exactly. I was planning to include this initially in the RFC but I decided to leave it out eventually, mostly to avoid expanding the scope.
There was a problem hiding this comment.
This baseline approach is also used by rubocop. The problem with these approaches is the baseline files have to be managed by humans and kept up to date.
As we built - our hold-the-line feature built into trunk check. Effectively does this dynamically. So nothing has to be maintained by humans as rules get enabled. Everything get's grandfathered in by any tool that integrates with our runner.
There was a problem hiding this comment.
- @EliSchleifer It's impressive but I don't like it. Means the user doesn't have insight into pre-existing issues and cannot work to make the issues go away (make the baseline smaller, pay technical debt). Unless I'm missing something you either make the tool to have to look at diffs for the rest of your life, or turn the feature off one day and fix all the pre-existing issues at once (which will never happen).
There was a problem hiding this comment.
I think we've learned from --cache that people will want to have the ability to rename this or place it in a different location. I'd say let's include that capability from the start.
There was a problem hiding this comment.
@nzakas I will make the necessary changes to the RFC to introduce two new flags --baseline and --baseline-location (instead of one flag called --generate-baseline), so that we maintain consistency with cache flags.
This should allow developers also to skip baseline if needed.
|
|
||
| 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. |
There was a problem hiding this comment.
This looks like a relative path. What is it relative to?
There was a problem hiding this comment.
Good point. Having a better look at this, it seems that it will better to convert this to a full path.
There was a problem hiding this comment.
This was changed to a relative path to CWD for portability reasons.
|
|
||
| ### 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: |
There was a problem hiding this comment.
I think we've learned from --cache that people will want to have the ability to rename this or place it in a different location. I'd say let's include that capability from the start.
|
|
||
| ### Caching | ||
|
|
||
| Caching must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: |
There was a problem hiding this comment.
What caching does this refer to? The --cache-generated file?
There was a problem hiding this comment.
Yes, it refers to the cache created/used when --cache is specified.
| Explain the design with enough detail that someone familiar with ESLint | ||
| can implement it by reading this document. Please get into specifics | ||
| of your approach, corner cases, and examples of how the change will be | ||
| used. Be sure to define any new terms in this section. |
There was a problem hiding this comment.
It seems like you missed the details this comment is asking for. Please look into the code to see how you'd implement such a feature.
| 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. |
There was a problem hiding this comment.
I think the error count is fine for a first pass at this feature.
|
|
||
| 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 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. |
There was a problem hiding this comment.
Just to clarify: we are only tracking problems set to the "error" severity and not anything set to "warn"? If so, please add this into the FAQ section.
There was a problem hiding this comment.
Correct, we are only counting errors when generating the baseline. Also only errors are considered when checking against the baseline.
| @@ -33,26 +33,32 @@ 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. | |||
There was a problem hiding this comment.
If we use an absolute path, then that means the file can't be checked in because it won't necessarily line up on other systems. Is that okay?
There was a problem hiding this comment.
No, that would be problematic. Reverting back to the concept of relative paths for portability reasons, where the paths are relative to the CWD. This should cover well cases where multiple paths/globs are provided and seem consistent with other parts.
There was a problem hiding this comment.
Perhaps relative to the flat config file is better?
The CWD can be somewhat problematic given that you may run eslint from any CWD and it'll figure things out as best as it can.
There was a problem hiding this comment.
I think defaulting to CWD-relative makes sense. People are probably going to run ESLint from the same location each time (the project root).
The flat config file may not always be in the CWD ancestry (if they use -c for instance), so that's unreliable.
|
|
||
| ``` 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. |
There was a problem hiding this comment.
Is there a reason generateBaseline() would need to be in eslint.js? (Note: We aren't making any further changes to eslint-legacy.js)
It seems like the functionality to generate a baseline could be encapsulated in a single function that takes a result set and returns the baseline format?
Thinking this through, it might actually be better to create a ResultBaseline (or some other rational name) class that can both generate a baseline and validate results against a baseline.
What do you think?
There was a problem hiding this comment.
The reason this was suggested was mostly to be aligned with fix and others i.e. outputFixes . I like the suggested approach though, this should facilitate testing too. I am outlining below a possible implementation:
/**
* @typedef {Record<string, { count: number }>} BaselineResult
*/
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 should happen outside of the above class. An idea is to make getCacheFile a bit more abstract so that we could 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.
Levaraging the above the changes in cli.js should look something like:
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;
}Many thanks for your feedback btw!! Let me know I should include the above examples in the RFC.
There was a problem hiding this comment.
Yeah, I think that's a good starting point. We can iterate more on the API once it's included.
|
|
||
| ``` 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. |
There was a problem hiding this comment.
cli-engine is deprecated. We would implement this feature only with flat config format.
|
@octogonz Would really like your feedback on this based on your work on Rush Stack Bulk. |
|
The feature that we designed is part of @rushstack/eslint-patch. Kevin Yang summarized it in this blog post. A key design question is how to keep bulk suppressions stable across Git merges, since they are stored separately from the associated source code. From a cursory glance, it sounds like this RFC is proposing to count the number of errors per file path. We prototyped that design, as well as a design that tracks line #'s, and also a design that tracks formatted messages ( Ultimately we settled on a JSON file using a There's probably still a lot that can be improved about our approach. However I can say that Microsoft and TikTok are using it successfully in two very large TypeScript monorepos, where style guide migrations involve hundreds of thousands of source files and megabytes of JSON. So it's worth studying even if you don't adopt it. 🙂 As far as future improvements, a feature I'd like to see is an API that enables the VS Code extension to highlight suppressed violations specially. Currently people either highlight them as normal errors (which I find confusing, since you have to run the CLI to find out whether you really need to fix it or not) or else don't highlight them at all. |
|
@octogonz thanks for the insights. I think we're past the point of adopting eslint-bulk wholesale, but would welcome your feedback on this RFC to see if there are any salient points that have been missed or problem areas you've encountered. |
Agreed, the I've been really busy lately, but I'd like to help out with the RFC. Let me make some time to read through @softius's doc in more detail, then follow up with feedback. |
|
@softius just a reminder that there are some suggestions for you to apply. Please, let us know if you need any help. |
|
I really support this RFC - we've been running our own baseline package (just a wrapper around eslint), which takes the same approach. We've been using this implementation on about 30+ prod projects with no complaints from our team. Hopefully, this implementation can assist with the RFC |
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
There was a problem hiding this comment.
Apologies for the delay in getting back to this. Had a lot of post-v9 work.
I think there are some helpful ideas that eslint-bulk has that we should consider here:
- Using the term "suppressions" instead of "baseline" - I think this clarifies what is actually happening. While people may not understand what generating a baseline is, most will understand when something is suppressed.
- A way to indicate which rules should be suppressed. For instance, if I just want to suppress "no-eval", I should be able to do that. That may mean rethinking the command line flags. So maybe
--suppress-rule <ruleId>and--suppress-allfor all rules?
|
|
||
| ### 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: |
There was a problem hiding this comment.
To make our lives easier, I think .eslint-baseline.json would be a better name. Easier to read, easier to edit with a known file extension.
|
|
||
| ``` 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. |
There was a problem hiding this comment.
Yeah, I think that's a good starting point. We can iterate more on the API once it's included.
|
@Humni that's great! Do you have any specific feedback on the implementation described in this proposal? How does it compare to what you're already using? |
|
Hi Nicholas, thanks for checking out the eslint-bulk project!
Other names we considered were exemptions, pardons, and legacy-suppressions. On a semi-related note, the "baselines" feature seems to very closely resemble the already existing eslint --max-warnings flag, which might be a source of confusion if the two features are both retained. |
@kevin-y-ang I'm not sure I follow. Can you explain what you mean by this? |
|
At a conceptual level, both the proposed baseline feature and the --max-warnings feature set a threshold of X errors where the system will alarm if there are >X errors. At our company, we previously used The message essentially being conveyed here is "Okay there are already X number of ESLint warnings in this package but we won't allow any more", while the message being conveyed with the baseline feature seems to be a more granular version of this: "Okay there are already X number of ESLint errors/warnings for rule R in file F but we won't allow any more." Anyway, it's just a similarity I see, it's not a big deal. |
Probably the main decision to make for this RFC which style of error matching you use. These would be:
With our current implementation, we use the Exact Error Line Matching, which does have some short falls. It works pretty well, the vast majority of PRs don't require any rework. There are cases where if you modify a line at the top of a file though, will cause all of the existing errors to be exposed. This approach was chosen due to simplicity. For the most part this does push a project to having a smaller and smaller baseline, however it does lead to developers touching the baseline "too much" due to them not wanting to fix errors not directly related to their PR. We're currently exploring moving to a context aware error matching, so it only comes up if the nearest 3 lines of code are modified - LuminateOne/eslint-baseline#8. This would provide a solution to the issue above, but it's not yet tested but seems like a sensible approach (if it can be done in a performant way). I would highly recommend an error count approach the same as PHPStan (as per comments from @ondrejmirtes), as that seems to sit with the development team better. Only other question "feature request" I would add into this is to allow for "flushing" a baseline of excess/resolved errors only (and not add any new ones). Just a QOL improvement though as it's typically easy enough to see new lines added into the diff in the baseline. |
|
@Humni gotcha, that's helpful. So it sounds like the current RFC, which uses error counting, is what you'd prefer. 👍 Keep in mind, too, that we generally build things incrementally. Our goal here is to get a baseline (no pun intended) of support for suppressions such that we can continue to build on top of it as feedback comes in. |
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
errorand be notified when new ones show up.Related Issues