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.

Sign up for GitHub

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

Jump to bottom

feat: Introduce baseline system to suppress existing errors #119

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+220 −0
Open

feat: Introduce baseline system to suppress existing errors #119

Changes from 3 commits
Commits
Show all changes
6 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
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 13 additions & 5 deletions designs/2024-baseline-support/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think the error count is fine for a first pass at this feature.


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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


```
{
"src/app/components/foobar/foobar.component.ts": {
"/home/user/project/src/app/components/foobar/foobar.component.ts": {
"@typescript-eslint/no-explicit-any": 1
Copy link
Member

Choose a reason for hiding this comment

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

To allow for extension in the future, I'd recommend using an object:

Suggested change
"@typescript-eslint/no-explicit-any": 1
"@typescript-eslint/no-explicit-any": {
count: 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:
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.
Expand Down Expand Up @@ -187,7 +193,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

Expand Down