feat: Add new includeIgnoreFile() to config-helpers#430
feat: Add new includeIgnoreFile() to config-helpers#430kirkwaiblinger wants to merge 35 commits intoeslint:mainfrom
includeIgnoreFile() to config-helpers#430Conversation
includeIgnoreFIle() to config-helpersincludeIgnoreFile() to config-helpers
| }); | ||
|
|
||
| it("should return an object with an `ignores` property", () => { | ||
| const ignoreFilePath = fileURLToPath( |
There was a problem hiding this comment.
I think we should simplify this and in other spots by using import.meta.resolve (requires Node v18+)
| const ignoreFilePath = fileURLToPath( | |
| const ignoreFilePath = import.meta.resolve("../tests/fixtures/ignore-files/gitignore1.txt") |
There was a problem hiding this comment.
Huh... I never knew about import.meta.resolve. But in any case it looks like that still gets us a URL, which would need to be converted to a path with fileURLToPath(). So I'm not sure that that gains us anything? Unless I'm misunderstanding.
At the end of the day, this is really just the workaround for not having access to import.meta.dirname, which the linter complains about (due to supporting older versions of node)
| // functionality of the `includeIgnoreFile` in `@eslint/compat`. The only | ||
| // discrepancy is the default name has been changed to reflect that it is | ||
| // really eslintignore mode by default. | ||
| describe("`includeIgnoreFile` compat with @eslint/compat", () => { |
There was a problem hiding this comment.
The tests for the object form options ({ mode: string, name: string }) seem to be missing?
There was a problem hiding this comment.
This section of tests was copied verbatim from the @eslint/compat repo (with the one noted change). That version of includeIgnoreFile only supports a string for its second argument.
There was a problem hiding this comment.
Oh, I see, though, it looks like I haven't added a test with that combination in the preceding section. I'll go ahead and add one 👍
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks! Leaving open for others to verify.
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
There was a problem hiding this comment.
Pull request overview
This PR moves ignore-file utilities into @eslint/config-helpers by adding includeIgnoreFile() (with gitignore-relative support and multi-file support) and re-exporting convertIgnorePatternToMinimatch(), while deprecating the equivalents in @eslint/compat and updating @eslint/migrate-config to consume the new export.
Changes:
- Add
includeIgnoreFile()+convertIgnorePatternToMinimatch()implementation, docs, fixtures, and unit tests to@eslint/config-helpers. - Update
@eslint/migrate-configto importconvertIgnorePatternToMinimatchfrom@eslint/config-helpersand adjust dependencies. - Deprecate
@eslint/compatignore-file helpers and document the deprecation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/migrate-config/src/migrate-config.js | Switches convertIgnorePatternToMinimatch import to @eslint/config-helpers. |
| packages/migrate-config/package.json | Replaces @eslint/compat dependency with @eslint/config-helpers; removes eslint devDependency. |
| packages/config-helpers/tests/types/types.test.ts | Adds TS type-surface checks for includeIgnoreFile() overload/return types. |
| packages/config-helpers/tests/ignore-file.test.js | Adds unit tests for includeIgnoreFile() and convertIgnorePatternToMinimatch(). |
| packages/config-helpers/tests/fixtures/ignore-files/gitignore1.txt | Adds fixture ignore file used by new unit tests. |
| packages/config-helpers/src/index.js | Re-exports new ignore-file utilities from the package entrypoint. |
| packages/config-helpers/src/ignore-file.js | Implements includeIgnoreFile() (multi-file + basePath option) and convertIgnorePatternToMinimatch(). |
| packages/config-helpers/README.md | Documents includeIgnoreFile() usage, options, and multi-file behavior. |
| packages/compat/tests/ignore-file.test.js | Updates file header comment (test behavior unchanged). |
| packages/compat/src/ignore-file.js | Marks compat helpers as deprecated in JSDoc. |
| packages/compat/README.md | Adds deprecation notice for compat includeIgnoreFile() documentation section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Francesco Trotta <github@fasttime.org>
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks! Leaving open for others to verify their review comments.
| throw new Error( | ||
| "The options argument to `includeIgnoreFile()` should be an object or a string.", | ||
| ); | ||
| } | ||
|
|
||
| const gitignoreResolution = optionsObject.gitignoreResolution ?? false; | ||
| if (typeof gitignoreResolution !== "boolean") { | ||
| throw new Error( | ||
| "The `gitignoreResolution` option must be specified a boolean or omitted", | ||
| ); | ||
| } | ||
|
|
||
| const name = optionsObject.name ?? `Imported .gitignore patterns`; | ||
| if (typeof name !== "string") { | ||
| throw new Error( |
There was a problem hiding this comment.
| throw new Error( | |
| "The options argument to `includeIgnoreFile()` should be an object or a string.", | |
| ); | |
| } | |
| const gitignoreResolution = optionsObject.gitignoreResolution ?? false; | |
| if (typeof gitignoreResolution !== "boolean") { | |
| throw new Error( | |
| "The `gitignoreResolution` option must be specified a boolean or omitted", | |
| ); | |
| } | |
| const name = optionsObject.name ?? `Imported .gitignore patterns`; | |
| if (typeof name !== "string") { | |
| throw new Error( | |
| throw new TypeError( | |
| "The options argument to `includeIgnoreFile()` should be an object or a string.", | |
| ); | |
| } | |
| const gitignoreResolution = optionsObject.gitignoreResolution ?? false; | |
| if (typeof gitignoreResolution !== "boolean") { | |
| throw new TypeError( | |
| "The `gitignoreResolution` option must be specified a boolean or omitted", | |
| ); | |
| } | |
| const name = optionsObject.name ?? `Imported .gitignore patterns`; | |
| if (typeof name !== "string") { | |
| throw new TypeError( |
Maybe TypeError would be more specific here? When checking an argument’s type, TypeError seems more precise, and it looks like other places follow the TypeError approach for argument validation as well.
rewrite/packages/config-helpers/src/global-ignores.js
Lines 30 to 36 in c872ca8
It’s an optional suggestion, but I think adding a TypeError check to the tests would be a nice-to-have here.
assert.throws(
() => includeIgnoreFile(123),
{
name: "TypeError",
message: /error message/,
},
);| throw new Error( | ||
| "The first argument to `includeIgnoreFile()` should be a string or array of strings", | ||
| ); | ||
| } | ||
| if (!path.isAbsolute(ignorePath)) { | ||
| throw new Error( |
There was a problem hiding this comment.
| throw new Error( | |
| "The first argument to `includeIgnoreFile()` should be a string or array of strings", | |
| ); | |
| } | |
| if (!path.isAbsolute(ignorePath)) { | |
| throw new Error( | |
| throw new TypeError( | |
| "The first argument to `includeIgnoreFile()` should be a string or array of strings", | |
| ); | |
| } | |
| if (!path.isAbsolute(ignorePath)) { | |
| throw new TypeError( |
Same here :)
| const ignoreFilePath = fileURLToPath( | ||
| new URL( | ||
| "../tests/fixtures/ignore-files/gitignore1.txt", | ||
| import.meta.url, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Would it make sense to extract ignoreFilePath into a top-level helper variable here?
It looks like ignoreFilePath is used six times across the tests, so I think moving it could make the code a bit more concise.
| import.meta.url, | ||
| ), | ||
| ); | ||
| const basePath = fileURLToPath( |
There was a problem hiding this comment.
Moving basePath to the top level could also be a nice option, since it’s used twice (though I’m not strongly opinionated about it).
Prerequisites checklist
AI acknowledgment
What is the purpose of this pull request?
fixes #329
What changes did you make? (Give an overview)
Deprecate
includeIgnoreFile(),convertIgnorePatternToMinimatch()in @eslint/compat. Add and export newincludeIgnoreFile()(documented),convertIgnorePatternToMinimatch()(undocumented) in @eslint/config-helpers.The new
convertIgnorePatternToMinimatch()is unaltered.The new
includeIgnoreFile():gitignoreResolutionoption which can befalse(default; interprets patterns relative to config file) ortrue(interprets patterns relative to ignore file).Related Issues
#329
Is there anything you'd like reviewers to focus on?
There is a companion PR in eslint repo: eslint/eslint#20735