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

fix: handle files with unspecified path in getRulesMetaForResults #16437

Merged
merged 4 commits into from Oct 31, 2022

Conversation

fasttime
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #16410

What changes did you make? (Give an overview)

  • In getRulesMetaForResults, files with an unspecified path are now treated as if they were located inside cwd.
  • In getRulesMetaForResults, when a result referencing a rule has no config, we will explicitly throw an error with a descriptive message.
  • Added top-level, internal functions getPlaceholderPath and createExtraneousResultsError to avoid code repetition.
  • Added two new unit tests.
  • Renamed an existing unit test to better disambiguate it from a new one. Also changed the assertion to check both error message and constructor.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Oct 17, 2022
@netlify
Copy link

netlify bot commented Oct 17, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 90b7df2
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/635d1277201d190008c3efb1

@fasttime fasttime marked this pull request as ready for review October 17, 2022 20:10
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 18, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, but would like @nzakas to take a look.

@mdjermanovic
Copy link
Member

Oh, and there's a merge conflict.

@fasttime
Copy link
Contributor Author

Thanks for reviewing @mdjermanovic! I've rebased the branch and fixed the merge conflict.

*/
const config = configs.getConfig(filePath);

if (!config) {
Copy link
Member

Choose a reason for hiding this comment

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

There may be an edge case here. getConfig returns undefined if a file is ignore by the configuration, so it doesn’t necessarily mean that the file wasn’t run in this instance. I think this will result in an error if the results contain a warning for a file that was ignored.

You can test this by passing in a file that is specifically ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also thought about that. Now that #16402 has been fixed, the specific edge case you mention should be handled correctly. I will rebase the branch once again and add a new unit test to check the behavior in case an explicitly ignored file appears in the results when warnIgnored is set. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new unit test for when an anonymous file is ignored and linted with warnIgnored. A similar case, where the file has a specified path, is already covered in another test:

const results = await engine.lintText("", { filePath: "ignored.js", warnIgnored: true });
const rulesMeta = engine.getRulesMetaForResults(results);
assert.deepStrictEqual(rulesMeta, {});

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 we should add a test with lintFiles() as well. We need to make sure that if there are multiple files linted and one ignored that this won't throw an error.

My concern: isFileIgnored() just does getConfig() === undefined, so what you are really testing here is if a file is ignored. Because of that, it still seems like there's an edge case here that will bite us.

I think it would be safe to remove this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignored file results, as they don't have any messages with a ruleId, should be skipped by the continue statement a few lines above.

The point of checking if a config is found here is to avoid results linted by a different engine. If a result was not linted by the current engine, it may have no config without being ignored. This specific case is covered in this test. If this check is removed, getRulesMetaForResults will still fail, but with a non-specific error message (because config.plugins cannot be accessed at some point).

Adding a test with linted files and an ignored file at the same time is a good idea. There are no tests with multiple results so far. Thanks for the suggestion!

* In `getRulesMetaForResults`, files with an unspecified path are now treated as if they were located inside `cwd`.
* In `getRulesMetaForResults`, when a result referencing a rule has no config, we will explicitly throw an error with a descriptive message.
* Added top-level, internal functions `getPlaceholderPath` and `createExtraneousResultsError` to avoid code repetition.
* Added two new unit tests.
* Renamed an existing unit test to better disambiguate it from a new one. Also changed the assertion to check both error message and constructor.

Fixes #16410
@fasttime
Copy link
Contributor Author

Rebased and updated as suggested by @nzakas. Since the new unit tests are only relevant to FlatESLint, I put them all at the start of the describe block, just under another existing, FlatESLint-specific unit test.

@fasttime
Copy link
Contributor Author

Added another unit test with lintFiles() as suggested by @nzakas.

});

const results = await engine.lintFiles(["missing-semicolon.js", "passing.js", "undef.js"]);
const rulesMeta = engine.getRulesMetaForResults(results);
Copy link
Member

Choose a reason for hiding this comment

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

Can we verify that a file is actually being ignored here before doing the other assertions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've added an assertion to make sure that one of the messages returned by lintFiles belongs to an ignored file.
I chose this implementation because it doesn't require calling another method on the engine under test.
Alternatively, we could pass the filePath of each result to isPathIgnored().

Copy link
Member

Choose a reason for hiding this comment

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

This works. 👍

nzakas
nzakas approved these changes Oct 31, 2022
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks so much for this.

@nzakas nzakas merged commit 886a038 into eslint:main Oct 31, 2022
21 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 10, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.26.0` -> `8.27.0`](https://renovatebot.com/diffs/npm/eslint/8.26.0/8.27.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.27.0`](https://github.com/eslint/eslint/releases/tag/v8.27.0)

[Compare Source](eslint/eslint@v8.26.0...v8.27.0)

#### Features

-   [`f14587c`](eslint/eslint@f14587c) feat: new `no-new-native-nonconstructor` rule ([#&#8203;16368](eslint/eslint#16368)) (Sosuke Suzuki)
-   [`978799b`](eslint/eslint@978799b) feat: add new rule `no-empty-static-block` ([#&#8203;16325](eslint/eslint#16325)) (Sosuke Suzuki)
-   [`69216ee`](eslint/eslint@69216ee) feat: no-empty suggest to add comment in empty BlockStatement ([#&#8203;16470](eslint/eslint#16470)) (Nitin Kumar)
-   [`319f0a5`](eslint/eslint@319f0a5) feat: use `context.languageOptions.ecmaVersion` in core rules ([#&#8203;16458](eslint/eslint#16458)) (Milos Djermanovic)

#### Bug Fixes

-   [`c3ce521`](eslint/eslint@c3ce521) fix: Ensure unmatched glob patterns throw an error ([#&#8203;16462](eslint/eslint#16462)) (Nicholas C. Zakas)
-   [`886a038`](eslint/eslint@886a038) fix: handle files with unspecified path in `getRulesMetaForResults` ([#&#8203;16437](eslint/eslint#16437)) (Francesco Trotta)

#### Documentation

-   [`ce93b42`](eslint/eslint@ce93b42) docs: Stylelint property-no-unknown ([#&#8203;16497](eslint/eslint#16497)) (Nick Schonning)
-   [`d2cecb4`](eslint/eslint@d2cecb4) docs: Stylelint declaration-block-no-shorthand-property-overrides ([#&#8203;16498](eslint/eslint#16498)) (Nick Schonning)
-   [`0a92805`](eslint/eslint@0a92805) docs: stylelint color-hex-case ([#&#8203;16496](eslint/eslint#16496)) (Nick Schonning)
-   [`74a5af4`](eslint/eslint@74a5af4) docs: fix stylelint error ([#&#8203;16491](eslint/eslint#16491)) (Milos Djermanovic)
-   [`324db1a`](eslint/eslint@324db1a) docs: explicit stylelint color related rules ([#&#8203;16465](eslint/eslint#16465)) (Nick Schonning)
-   [`94dc4f1`](eslint/eslint@94dc4f1) docs: use Stylelint for HTML files ([#&#8203;16468](eslint/eslint#16468)) (Nick Schonning)
-   [`cc6128d`](eslint/eslint@cc6128d) docs: enable stylelint declaration-block-no-duplicate-properties ([#&#8203;16466](eslint/eslint#16466)) (Nick Schonning)
-   [`d03a8bf`](eslint/eslint@d03a8bf) docs: Add heading to justification explanation ([#&#8203;16430](eslint/eslint#16430)) (Maritaria)
-   [`8a15968`](eslint/eslint@8a15968) docs: add Stylelint configuration and cleanup ([#&#8203;16379](eslint/eslint#16379)) (Nick Schonning)
-   [`9b0a469`](eslint/eslint@9b0a469) docs: note commit messages don't support scope ([#&#8203;16435](eslint/eslint#16435)) (Andy Edwards)
-   [`1581405`](eslint/eslint@1581405) docs: improve context.getScope() docs ([#&#8203;16417](eslint/eslint#16417)) (Ben Perlmutter)
-   [`b797149`](eslint/eslint@b797149) docs: update formatters template ([#&#8203;16454](eslint/eslint#16454)) (Milos Djermanovic)
-   [`5ac4de9`](eslint/eslint@5ac4de9) docs: fix link to formatters on the Core Concepts page ([#&#8203;16455](eslint/eslint#16455)) (Vladislav)
-   [`33313ef`](eslint/eslint@33313ef) docs: core-concepts: fix link to semi rule ([#&#8203;16453](eslint/eslint#16453)) (coderaiser)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjEuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1628
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly contributor pool core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: FlatESLint getRulesMetaForResults fails on anonymous files when option cwd is set
3 participants