fix: match glob logic for unignore ignores #115
Conversation
|
Hi @yidingww!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
yidingww
changed the title
yidingww
changed the title
shared/configs.ts
Outdated
| } | ||
| }) | ||
|
|
||
| return flatGlobs.filter(glob => !unmatched.includes(glob)) |
There was a problem hiding this comment.
I guess the order of ignore and unignore might matter how it work. For the must accurate result, we might need to vendor ESLint's walker here: https://github.com/eslint/eslint/blob/5db226f4da9ad7d53a4505a90290b68d4036c082/lib/eslint/eslint-helpers.js#L216
There was a problem hiding this comment.
@antfu I just updated the PR
I took a different approach, cos i realised that @eslint/config-array has a very convenient isFileIgnored here, we can use this to decide if a file is ignored, and that should fix the issue entirely.
I don't think we need to vendor the eslint-helper since it's not exported and there are a long chain of dependencies in eslint's implementations.
yidingww
force-pushed
the
fix/negate-glob
branch
from
0420d93 to
470d160
Compare
shared/configs.ts
Outdated
| // push negative globs except for unignore globs | ||
| result.globs.push(...negative.filter(glob => !glob.startsWith('!'))) |
There was a problem hiding this comment.
I think you might also want to push negative unignore globs here, basically all matched globs. There's a comment here where I made that switch, and I think including them causes the UI to be more clear to the user since it indicates that the globs matched, and the user can then interpret the "matched-unignore" (vs the unclear/inconsistent greyed out ui that implies it didn't match glob-wise)
| // only include ignores because of how ConfigArray works internally: | ||
| // isFileIgnored only works when only `ignore` exists | ||
| // (https://github.com/eslint/rewrite/blob/config-array-v0.19.1/packages/config-array/src/config-array.js#L726) | ||
| ignores: config.ignores ?? [], |
|
Fwiw, I also added tests that you could bring over if @antfu likes this
approach more. Would help make sure the functionality is identical too!
…On Sat, Dec 28, 2024, 08:16 Wang Yiding ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In shared/configs.ts
<#115 (comment)>
:
> @@ -48,17 +48,32 @@ export function matchFile(
}
configs.forEach((config, index) => {
+ const isFileGlobalIgnored = configArray.isFileIgnored(filepath)
+ let isFileIgnoredInCurrConfig = false
+ if (config.ignores) {
+ const ignoreConfigArray = buildConfigArray([{
+ index: config.index,
+ // only include ignores because of how ConfigArray works internally:
+ // isFileIgnored only works when only `ignore` exists
+ // (https://github.com/eslint/rewrite/blob/config-array-v0.19.1/packages/config-array/src/config-array.js#L726)
+ ignores: config.ignores ?? [],
@antfu <https://github.com/antfu> @comp615 <https://github.com/comp615>
Hi guys, I just updated the PR again here in this commit
On a special note, here in particular, we need to check the current config
as well to make sure the file is not ignored in current config. See inline
code comment for more details
—
Reply to this email directly, view it on GitHub
<#115 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEELRJ7IKXW7UYYCR6T4QD2H2QEPAVCNFSM6AAAAABUF5MR7GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRUG4YTGNBUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
fix #114
The logic of
getMatchedGlobsneeds to be updated:If an unignore glob is matched, and if this glob without the
!matches any previous ignore globs, all these globs should be unmatched.