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: Ensure flat config unignores work consistently like eslintrc #16579

Merged
merged 2 commits into from
Dec 16, 2022

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Nov 23, 2022

We have several tests that were ignored back when we were waiting for fast-glob fixes. Because we no longer use fast-glob, the tests now work.

Prerequisites checklist

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

[ ] Documentation update
[ ] 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
[x] Other, please explain:

Test updates

What changes did you make? (Give an overview)

There were several tests that we were ignoring for FlatESLint due to bugs in fast-glob. Now that we no longer use fast-glob, we can make these tests run.

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

I changed the last test a bit because ignores works a bit differently than ignorePatterns did. Please double-check that change.

@eslint-github-bot eslint-github-bot bot added chore This change is not user-facing triage An ESLint team member will look at this issue soon labels Nov 23, 2022
@netlify
Copy link

netlify bot commented Nov 23, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit e16e16a
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/639b8b3938dd940009ec3c8e


const { prepare, cleanup, getPath } = createCustomTeardown({
cwd: root,
cwd: `${root}-unignores`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like there was some timing issues where just root was colliding due to Mocha running some tests in parallel, so I made them unique.

path.join(root, "node_modules/foo/index.js")
path.join(getPath(), "eslint.config.js"),
path.join(getPath(), "foo.js"),
path.join(getPath(), "node_modules/foo/.dot.js"),
Copy link
Member Author

Choose a reason for hiding this comment

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

We now include dot files by default, so had to update tests accordingly.

"node_modules/myconf/eslint.config.js": `module.exports = {
ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
"node_modules/myconf/eslint.config.js": `module.exports = [{
ignores: ["!node_modules/myconf", "foo/*.js"],
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove "**/eslint.config.js" from this array. With ignorePatterns, that file would be ignored; with ignores, the file isn't ignored due to "!node_module/myconf unignoring the whole directory, which overrides "**/eslint.config.js".

ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
"node_modules/myconf/eslint.config.js": `module.exports = [{
ignores: ["!node_modules/myconf", "foo/*.js"],
}, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to separate into a different config object so ignores is treated as global ignores.

@mdjermanovic mdjermanovic added 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 Nov 24, 2022
Comment on lines 4509 to 4510
"eslint.config.js": `module.exports = {
ignores: ["!**/node_modules/foo/**"]
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect this pattern to have any effects after **/node_modules/**.

**/node_modules/**
!**/node_modules/foo/**

The first test below asserts that node_modules/foo/index.js is not ignored. I think that this file should be ignored because it's in ignored directory node_modules/foo.

**/node_modules/** ignores node_modules/foo, but !**/node_modules/foo/** does not unignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, are you sure about that? it seems like this test was indicating our behavior worked in a different way.

Generally we are treating patterns ending with star-str as file matchers and not directory matchers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the current test, which verifies this behavior:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L5413-L5460

Copy link
Member

Choose a reason for hiding this comment

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

Here is the current test, which verifies this behavior:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L5413-L5460

In the original test, the list of patterns is:

/**/node_modules/*
!/node_modules/foo

node_modules/ doesn't match any patterns; node_modules/foo/ matches /**/node_modules/* but also matches a subsequent negative pattern !/node_modules/foo so it's not ignored; node_modules/foo/index.js doesn't match any patterns.

In the flat eslint test, the list of patterns is:

**/node_modules/**
!**/node_modules/foo/**

node_modules/ doesn't match any patterns; node_modules/foo/ matches **/node_modules/** and doesn't match any negative patterns so node_modules/foo/ is ignored and thus node_modules/foo/index.js should be ignored because it's in an ignored directory.

Copy link
Member

Choose a reason for hiding this comment

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

Generally we are treating patterns ending with star-str as file matchers and not directory matchers.

I think it would be confusing if trailing /** doesn't match subdirectories, as it is expected to match everything.

In my opinion, foo/** pattern should not match foo/ directory, but it should match foo/bar/ directory. This is how gitignore works. In minimatch, however, foo/** does match foo/ directory, but that may be a bug (isaacs/minimatch#179).

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like that minimatch behavior is intentional.

I'll take some time to look at this again...it's such an edge case that I think there's room for disagreement on how to handle this. Because we are not explicitly guaranteeing to work the same way gitignore works, I think we can kind of chart our own course here.

Copy link
Member Author

Choose a reason for hiding this comment

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

So interestingly, it appears that ConfigArray actually works the way you describe. I added some tests to verify:
humanwhocodes/config-array#73

It looks like there must be a bug on the ESLint side of things, so I'm investigating that next.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look at humanwhocodes/config-array#73.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it actually looks like it's a bug in isDirectoryIgnored() as opposed to isFileIgnored().

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix is up, for when you have time:
humanwhocodes/config-array#74

"node_modules/myconf/eslint.config.js": `module.exports = {
ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
"node_modules/myconf/eslint.config.js": `module.exports = [{
ignores: ["!node_modules/myconf", "foo/*.js"],
Copy link
Member

Choose a reason for hiding this comment

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

!node_modules/myconf also shouldn't have any effects after **/node_modules/**. because it unignores the directory but everything under it is still ignored by **/node_modules/**.

Copy link
Member Author

Choose a reason for hiding this comment

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

This also seems counter to what existing tests are indicating. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the original test:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L6426-L6459

It seems to behave in the same way as I have currently.

Copy link
Member

Choose a reason for hiding this comment

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

Here is the original test: https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L6426-L6459

This is a bit different because in eslintrc the default pattern is /**/node_modules/* (* instead of ** at the end is the difference; leading slash doesn't matter), so in the original test the list of patterns is:

/**/node_modules/*
!/node_modules/myconf

node_modules/ doesn't match any patterns; node_modules/myconf/ matches /**/node_modules/* but also matches a subsequent negative pattern !/node_modules/myconf so it's not ignored; node_modules/myconf/foo/ doesn't match any patterns; node_modules/myconf/foo/test.js doesn't match any patterns.

In the flat eslint test, the list of patterns is:

**/node_modules/**
!node_modules/myconf

node_modules/ doesn't match any patterns; node_modules/myconf/ matches **/node_modules/* but also matches a subsequent negative pattern !node_modules/myconf so it's not ignored; node_modules/myconf/foo/ matches **/node_modules/** and doesn't match any negative patterns so that should be the end - directory node_modules/myconf/foo/ is ignored and thus everything under it is ignored as well, including node_modules/myconf/foo/test.js.

@Gstar123456

This comment was marked as spam.

@Gstar123456

This comment was marked as spam.

@eslint eslint deleted a comment from Gstar123456 Nov 25, 2022
@nzakas nzakas changed the title chore: Finish FlatESLint tests fix: Ensure flat config unignores work consistently like eslintrc Dec 14, 2022
@nzakas nzakas added bug ESLint is working incorrectly and removed chore This change is not user-facing labels Dec 14, 2022
@nzakas nzakas requested a review from a team as a code owner December 14, 2022 21:00
@nzakas
Copy link
Member Author

nzakas commented Dec 14, 2022

After digging deeply into this, it appears that this wasn't just a chore, but actually we needed a bug fix, so I've switched this PR to "fix".

I've updated the default ignore pattern for flat config to be **/node_modules/* to match what we have for eslintrc. This, plus an update of config-array now makes flat config unignores work similarly to eslintrc.

@nzakas
Copy link
Member Author

nzakas commented Dec 14, 2022

Hmmmm, it looks like tests are failing on Node.js 19 only...no idea why that would be. I even downloaded Node.js 19 locally and can't reproduce. 🤔 No more time to look into this today, so if anyone has any ideas, I'd appreciate the help.

@mdjermanovic
Copy link
Member

mdjermanovic commented Dec 14, 2022

Seems that the problem is file: with recently released npm v9.2.0 which is included in recently released Node v19.3.0.

@mdjermanovic
Copy link
Member

This is actually npm major upgrade v8 -> v9, so there must be a breaking change in v9 that's causing this. On the latest working commit it was { node: 'v19.2.0', npm: '8.19.3' }, now it is { node: 'v19.3.0', npm: '9.2.0' }.

@mdjermanovic
Copy link
Member

it looks like tests are failing on Node.js 19 only

Fixed on main now.

@mdjermanovic
Copy link
Member

There's a merge conflict now.

We have several tests that were ignored back when we were waiting for
fast-glob fixes. Because we no longer use fast-glob, the tests now work.
@nzakas
Copy link
Member Author

nzakas commented Dec 15, 2022

Thanks for fixing that! I've rebased and resolved the merge conflicts.

@mdjermanovic
Copy link
Member

docs/** pattern now ignores docs/ directory? This is observable from running npm run lint:docsjs ( runs eslint docs/.eleventy.js):

C:\milos\eslint\docs\.eleventy.js
  0:0  warning  File ignored by default.  Use a negated ignore pattern (like "--ignore-pattern '!<relative/path/to/filename>'") to override

✖ 1 problem (0 errors, 1 warning)

I think this is good, because this is how minimatch works, and whatever we try to adjust in minimatch's behavior probably won't work 100% correctly, just wanted to check with you if this is intentional because config-array docs still state that it shouldn't match, here.

@nzakas
Copy link
Member Author

nzakas commented Dec 15, 2022

Oops yes, that is intentional. I just forgot to update the config-array docs:
humanwhocodes/config-array#76

beforeEach(prepare);
afterEach(cleanup);

it("'isPathIgnored()' should return 'true' for 'node_modules/foo/index.js'.", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it("'isPathIgnored()' should return 'true' for 'node_modules/foo/index.js'.", async () => {
it("'isPathIgnored()' should return 'false' for 'node_modules/foo/index.js'.", async () => {

Test title doesn't match the assertion in the test.

The assertion is correct. node_modules/foo/ is ignored by default, but since **/node_modules/foo/** now matches node_modules/foo/, !**/node_modules/foo/** will unignore it, so the files inside are not ignored.

assert.strictEqual(await engine.isPathIgnored("node_modules/foo/index.js"), false);
});

it("'isPathIgnored()' should return 'true' for 'node_modules/foo/.dot.js'.", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it("'isPathIgnored()' should return 'true' for 'node_modules/foo/.dot.js'.", async () => {
it("'isPathIgnored()' should return 'false' for 'node_modules/foo/.dot.js'.", async () => {

Same as the previous, the test is correct just the title should be updated.

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, thanks! I'll prepare a follow-up to fix #16579 (comment) and to update ignore patterns in our config file. Not a blocker for the release.

@mdjermanovic mdjermanovic merged commit 1a327aa into main Dec 16, 2022
@mdjermanovic mdjermanovic deleted the flat-eslint-tests branch December 16, 2022 22:32
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 24, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.29.0...v8.30.0)

#### Features

-   [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#&#8203;16637](eslint/eslint#16637)) (Daniel Bartholomae)
-   [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#&#8203;16654](eslint/eslint#16654)) (Sébastien Règne)

#### Bug Fixes

-   [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#&#8203;16579](eslint/eslint#16579)) (Nicholas C. Zakas)
-   [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#&#8203;16611](eslint/eslint#16611)) (Milos Djermanovic)

#### Documentation

-   [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#&#8203;16663](eslint/eslint#16663)) (Nicholas C. Zakas)
-   [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#&#8203;16563](eslint/eslint#16563)) (Ben Perlmutter)
-   [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#&#8203;16606](eslint/eslint#16606)) (Sam Chen)
-   [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#&#8203;16631](eslint/eslint#16631)) (Percy Ma)
-   [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#&#8203;16625](eslint/eslint#16625)) (Milos Djermanovic)
-   [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#&#8203;16628](eslint/eslint#16628)) (Karl Horky)
-   [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#&#8203;16591](eslint/eslint#16591)) (Tanuj Kanti)
-   [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#&#8203;16566](eslint/eslint#16566)) (Ben Perlmutter)
-   [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#&#8203;16607](eslint/eslint#16607)) (Pavel)
-   [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#&#8203;16603](eslint/eslint#16603)) (Sam Chen)

#### Chores

-   [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.0 ([#&#8203;16675](eslint/eslint#16675)) (Milos Djermanovic)
-   [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#&#8203;14827](eslint/eslint#14827) ([#&#8203;16315](eslint/eslint#16315)) (Patrick McElhaney)
-   [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#&#8203;16664](eslint/eslint#16664)) (Milos Djermanovic)
-   [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#&#8203;16618](eslint/eslint#16618)) (Bryan Mishkin)

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC40IiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689
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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 15, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants