fix: match glob logic for unignore ignores #115

yidingww · 2024-12-25T12:11:25Z

The logic of getMatchedGlobs needs 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.

eslint-github-bot · 2024-12-25T12:11:30Z

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.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.

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

linux-foundation-easycla · 2024-12-25T12:11:33Z

The committers listed above are authorized under a signed CLA.

✅ login: yidingww / name: Wang Yiding (fc2ea9a, d5e0321, 0627a11, 470d160, 06c81e7, 12ca09e)
✅ login: antfu / name: Anthony Fu (e49cb74)
✅ login: comp615 / name: Charlie Croom (8e8eaf5, bed49b3, 9abdd0a, 5e60eb6)

yidingww · 2024-12-25T12:16:27Z

Probably a duplicate of #109, but i do hope you can take a look at this one as well @antfu

antfu · 2024-12-26T09:11:49Z

shared/configs.ts

+    }
+  })
+
+  return flatGlobs.filter(glob => !unmatched.includes(glob))


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

@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.

This reverts commit 0627a11.

comp615 · 2024-12-27T21:16:56Z

shared/configs.ts

+    // push negative globs except for unignore globs
+    result.globs.push(...negative.filter(glob => !glob.startsWith('!')))


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)

yidingww · 2024-12-28T13:16:33Z

shared/configs.ts

+        // 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 @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

comp615 · 2024-12-28T13:39:54Z

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: ***@***.***>

yidingww · 2024-12-29T18:00:44Z

@comp615 I cherry picked your commits over, and updated the solution again, now should be quite accurate

yidingww · 2024-12-30T02:38:11Z

patches/@eslint__config-array@0.19.1.patch

+  * @type {Set<string>}
+  */
+-const META_FIELDS = new Set(["name"]);
+const META_FIELDS = new Set(["name", "index"]);


The patch here is for @eslint/config-array to work as expected. Currently @eslint/config-array doesn't allow custome META_FIELDS so a patch is needed

I think this is the riskiest part of the PR. As a library maintainer, I probably wouldn't allow it. As a user, I'm all for it :-P

It looks like the upstream PR was closed here: eslint/rewrite#144, so I think it's important to make sure that there's actually a path towards the main-line using this approach. if that author refuses to allow something like this, then we're probably screwed here and need to find a different approach.

antfu · 2024-12-30T03:42:15Z

Thanks for both of your hard works! @comp615 @yidingww

This implementation looks more solid to me, @comp615 do you mind me taking this inside of #109? Or do you have any thing to add?

comp615 · 2025-01-02T14:40:49Z

app/pages/configs.vue

@@ -339,7 +339,7 @@ onMounted(async () => {
          <div>Ignored by globs:</div>
          <div flex="~ gap-2 items-center wrap">
            <GlobItem
-              v-for="glob, idx of fileMatchResult.globs"
+              v-for="glob, idx of fileMatchResult.globs.filter(glob => !glob.startsWith('!'))"


I think we still want to show negative globs here, they are part of the match so again we can let the user figure them out. I think it's most helpful to show everything that matches (positive or negative)

comp615 · 2025-01-02T14:44:50Z

patches/@eslint__config-array@0.19.1.patch

+  * @type {Set<string>}
+  */
+-const META_FIELDS = new Set(["name"]);
+const META_FIELDS = new Set(["name", "index"]);


I think this is the riskiest part of the PR. As a library maintainer, I probably wouldn't allow it. As a user, I'm all for it :-P

It looks like the upstream PR was closed here: eslint/rewrite#144, so I think it's important to make sure that there's actually a path towards the main-line using this approach. if that author refuses to allow something like this, then we're probably screwed here and need to find a different approach.

comp615 · 2025-01-02T14:46:09Z

shared/configs.ts

+  return globs.filter((glob) => {
+    const matchResult = minimatch(file, glob)
+    // for unignore glob, we need to flip back the match
+    return glob.startsWith('!') ? !matchResult : matchResult
+  }).flat()


IMPORTANT: Instead of doing this, see my patch where you should should add flipNegate: true to the minimatchOpts

comp615 · 2025-01-02T15:12:28Z

Thanks for both of your hard works! @comp615 @yidingww

This implementation looks more solid to me, @comp615 do you mind me taking this inside of #109? Or do you have any thing to add?

Agree that using the actual config files is a more directly and reliable way to match. I don't know the history here, but I assume something like the metadata issue is why it wasn't done this way in the first place? Or perhaps because it seems duplicative to match, and then match globs again anyways.

I left a little bit of feedback to clean it up; if I were you, I'd just want to make sure that there's a path forward to removing the "patch" of config-array. But otherwise I think this is great!

fix: match glob logic for negate ignores

0627a11

yidingww changed the title ~~fix #114: match glob logic for negate ignores~~ fix: match glob logic for negate ignores Dec 25, 2024

eslint-github-bot bot added the bug Something isn't working label Dec 25, 2024

yidingww marked this pull request as ready for review December 25, 2024 12:14

yidingww changed the title ~~fix: match glob logic for negate ignores~~ fix: match glob logic for unignore ignores Dec 25, 2024

antfu reviewed Dec 26, 2024

View reviewed changes

yidingww added 2 commits December 26, 2024 23:31

Revert "fix: match glob logic for negate ignores"

fc2ea9a

This reverts commit 0627a11.

fix: use config array built in ignore check

470d160

yidingww force-pushed the fix/negate-glob branch from 0420d93 to 470d160 Compare December 26, 2024 16:02

comp615 reviewed Dec 27, 2024

View reviewed changes

feat: check current config ignores as well

d5e0321

yidingww commented Dec 28, 2024

View reviewed changes

yidingww and others added 6 commits December 30, 2024 01:58

fix: make full use of config array

12ca09e

Add testing

9abdd0a

Revert config

bed49b3

little clean

5e60eb6

more clear

8e8eaf5

feat: update test and resolve conflicts

06c81e7

yidingww force-pushed the fix/negate-glob branch from 0f63dae to 06c81e7 Compare December 29, 2024 17:59

yidingww commented Dec 30, 2024

View reviewed changes

chore: migrate to Vitest

e49cb74

antfu approved these changes Dec 30, 2024

View reviewed changes

comp615 mentioned this pull request Jan 2, 2025

feat: allow unsafely customise META_FIELDS eslint/rewrite#144

Closed

1 task

comp615 reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: match glob logic for unignore ignores #115

fix: match glob logic for unignore ignores #115

yidingww commented Dec 25, 2024

eslint-github-bot bot commented Dec 25, 2024

linux-foundation-easycla bot commented Dec 25, 2024 •

edited

Loading

yidingww commented Dec 25, 2024

antfu Dec 26, 2024

yidingww Dec 26, 2024

comp615 Dec 27, 2024

yidingww Dec 28, 2024

comp615 commented Dec 28, 2024 via email

yidingww commented Dec 29, 2024

yidingww Dec 30, 2024

comp615 Jan 2, 2025

antfu commented Dec 30, 2024

comp615 Jan 2, 2025

comp615 Jan 2, 2025

comp615 Jan 2, 2025

comp615 commented Jan 2, 2025

		// push negative globs except for unignore globs
		result.globs.push(...negative.filter(glob => !glob.startsWith('!')))

fix: match glob logic for unignore ignores #115

Are you sure you want to change the base?

fix: match glob logic for unignore ignores #115

Conversation

yidingww commented Dec 25, 2024

eslint-github-bot bot commented Dec 25, 2024

linux-foundation-easycla bot commented Dec 25, 2024 • edited Loading

yidingww commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comp615 commented Dec 28, 2024 via email

yidingww commented Dec 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antfu commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comp615 commented Jan 2, 2025

linux-foundation-easycla bot commented Dec 25, 2024 •

edited

Loading