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: remove support for ignore files in FlatESLint #16355

Merged
merged 7 commits into from Oct 3, 2022
Merged

feat: remove support for ignore files in FlatESLint #16355

merged 7 commits into from Oct 3, 2022

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 27, 2022

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:

Refs #16264. Removes everything mentioned except for the ignorePatterns constructor option. I can do that in a separate PR when we are certain that the option should be removed.

What changes did you make? (Give an overview)

  • Removed all support for separate ignore files.
    • Removed loading .eslintignore files.
    • Removed ignorePath constructor option.
    • Removed --ignore-path CLI option.
  • Removed interpreting --ignore-patterns and ignorePatterns as .gitignore-style patterns.

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

@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 feature This change adds a new feature to ESLint labels Sep 27, 2022
@netlify
Copy link

netlify bot commented Sep 27, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ebaec07
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63336fa9d85290000ab1c728
😎 Deploy Preview https://deploy-preview-16355--docs-eslint.netlify.app/user-guide/configuring/configuration-files-new
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

* There is a separate command `target.lintDocsJS` for linting JavaScript files in the `docs` directory.
*/
echo("Validating JavaScript files");
lastReturn = exec(`${ESLINT}${fix ? "--fix" : ""} . --ignore-pattern docs`);
lastReturn = exec(`${ESLINT}${fix ? "--fix" : ""} . --ignore-pattern "docs/**"`);
Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this to be how we expect ignore patterns to work, but it would work the same without this change because of #16354.

Comment on lines 702 to +711
it(`should not process any files with configType:${configType}`, async () => {
const ignoredFile = getFixturePath("cli/syntax-error.js");
const ignorePathOption = useFlatConfig
? ""
: "--ignore-path .eslintignore_empty";
const filePath = getFixturePath("cli/passing.js");
const exit = await cli.execute(`--ignore-pattern cli/ ${ignoredFile} ${filePath}`, null, useFlatConfig);
const ignorePattern = useFlatConfig ? "cli/**" : "cli/";
const exit = await cli.execute(
`--ignore-pattern ${ignorePattern} ${ignorePathOption} ${ignoredFile} ${filePath}`, null, useFlatConfig
);
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 test was not working as intended because all the files were already ignored by .eslintignore, so --ignore-pattern actually had no effect.

Comment on lines -3267 to -3276
describe("with .eslintignore file or package.json file", () => {
it("should load .eslintignore from cwd when explicitly passed", async () => {
const cwd = getFixturePath("ignored-paths");
const engine = new FlatESLint({ cwd });

// `${cwd}/.eslintignore` includes `sampleignorepattern`.
assert(await engine.isPathIgnored("sampleignorepattern"));
});

});
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 test was not working as intended, because sampleignorepattern file does not match any config entries, so it was ignored regardless of .eslintignore.

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. Just want to give others a chance to review before merging.

"tmp/**",
"tools/internal-rules/node_modules/**",
"**/test.js",
"!**/.eslintrc.js"
Copy link
Member

Choose a reason for hiding this comment

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

Just double-checking: this still doesn't unignore .eslintrc.js correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. When doing a glob search with a pattern that doesn't have an explicit dot that matches leading dots in file names, .eslintrc.js will still be ignored because globby doesn't support negative patterns in ignore (in fact, I believe it removes leading ! so this actually ignores **/.eslintrc.js), and in general globby's dotfiles matching behavior seems to depend only on the content of search patterns.

On the other hand, when directly using our config array to evaluate whether or not .eslintrc.js is ignored (e.g., when passing a file path), it's not ignored regardless of this line. This behavior needs further analysis (#16265).

Overall, this line currently does not affect anything, but it may when we fix #16265 so I've left it in for now.

Copy link
Member

Choose a reason for hiding this comment

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

That’s what I figured. 👍

@nzakas nzakas merged commit 6964cb1 into main Oct 3, 2022
@nzakas nzakas deleted the issue16264 branch October 3, 2022 23:10
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 13, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.24.0...v8.25.0)

#### Features

-   [`173e820`](eslint/eslint@173e820) feat: Pass --max-warnings value to formatters ([#&#8203;16348](eslint/eslint#16348)) (Brandon Mills)
-   [`6964cb1`](eslint/eslint@6964cb1) feat: remove support for ignore files in FlatESLint ([#&#8203;16355](eslint/eslint#16355)) (Milos Djermanovic)
-   [`1cc4b3a`](eslint/eslint@1cc4b3a) feat: `id-length` counts graphemes instead of code units ([#&#8203;16321](eslint/eslint#16321)) (Sosuke Suzuki)

#### Documentation

-   [`90c6028`](eslint/eslint@90c6028) docs: Conflicting fixes ([#&#8203;16366](eslint/eslint#16366)) (Ben Perlmutter)
-   [`5a3fe70`](eslint/eslint@5a3fe70) docs: Add VS to integrations page ([#&#8203;16381](eslint/eslint#16381)) (Maria José Solano)
-   [`49bd1e5`](eslint/eslint@49bd1e5) docs: remove unused link definitions ([#&#8203;16376](eslint/eslint#16376)) (Nick Schonning)
-   [`3bd380d`](eslint/eslint@3bd380d) docs: typo cleanups for docs ([#&#8203;16374](eslint/eslint#16374)) (Nick Schonning)
-   [`b3a0837`](eslint/eslint@b3a0837) docs: remove duplicate words ([#&#8203;16378](eslint/eslint#16378)) (Nick Schonning)
-   [`a682562`](eslint/eslint@a682562) docs: add `BigInt` to `new-cap` docs ([#&#8203;16362](eslint/eslint#16362)) (Sosuke Suzuki)
-   [`f6d57fb`](eslint/eslint@f6d57fb) docs: Update docs README ([#&#8203;16352](eslint/eslint#16352)) (Ben Perlmutter)
-   [`7214347`](eslint/eslint@7214347) docs: fix logical-assignment-operators option typo ([#&#8203;16346](eslint/eslint#16346)) (Jonathan Wilsson)

#### Chores

-   [`1f78594`](eslint/eslint@1f78594) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).3.3 ([#&#8203;16397](eslint/eslint#16397)) (Milos Djermanovic)
-   [`8476a9b`](eslint/eslint@8476a9b) chore: Remove CODEOWNERS ([#&#8203;16375](eslint/eslint#16375)) (Nick Schonning)
-   [`720ff75`](eslint/eslint@720ff75) chore: use "ci" for Dependabot commit message ([#&#8203;16377](eslint/eslint#16377)) (Nick Schonning)
-   [`42f5479`](eslint/eslint@42f5479) chore: bump actions/stale from 5 to 6 ([#&#8203;16350](eslint/eslint#16350)) (dependabot\[bot])
-   [`e5e9e27`](eslint/eslint@e5e9e27) chore: remove `jsdoc` dev dependency ([#&#8203;16344](eslint/eslint#16344)) (Milos Djermanovic)

</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, click this checkbox.

---

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

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1577
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@jedwards1211
Copy link
Contributor

This surprised me and needs to be explicitly documented, #16433

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 2, 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 Apr 2, 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 core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants