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 unignore statements don't break file filtering #109

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

comp615
Copy link

@comp615 comp615 commented Dec 5, 2024

Fixes #63

Problem
ESLint supports "unignore patterns" in ignores statements. However due to the way the site handles matching today, these are treated as inverted glob matchs instead of special cases.

This means that any usage of an ignore pattern will be treated as if nothing except that pattern matches, effectively breaking everything

Solution
Use the flipNegate option to tell minimatch to not invert the matching, and then special cases such globs. Because we have a set of matches, we can basically assume that if the last match is a negation, then we don't have to ignore the file.

**/*.js
**/foo/*.js
!**/foo/specialjs

If another match had occurred after a negation, then it would still count.

Here, we add a function to handle these cases and apply them to the negative matching cases before returning the overall match status

Copy link

linux-foundation-easycla bot commented Dec 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@eslint-github-bot
Copy link

Hi @comp615!, 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'.
  • The first letter of the tag should be in lowercase

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

@comp615 comp615 changed the title Fix unignore statements breaking file filtering fix: ensure unignore statements don't break file filtering Dec 5, 2024
@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Dec 5, 2024
@comp615 comp615 force-pushed the ccroom/fix-unignore branch from 037e04f to aa9b5b7 Compare December 5, 2024 22:00
Comment on lines 38 to 43
/**
* Given a list of matched globs, if an unignore (leading !) is the last one, then the file no longer matches the glob set
*/
function filterUnignoreGlobs(globs: string[]) {
if (!globs.length || globs[globs.length - 1].startsWith('!'))
return []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if I follow, why would the unignore being the last makes the glob empty?

Copy link
Author

@comp615 comp615 Dec 6, 2024

Choose a reason for hiding this comment

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

I was primarily trying to make the change as small as possible within the existing code. Basically for the globalIgnore case...they are equivalent because if it's unignored, then we don't return globs anyways (i.e. the code carries on). It's by definition impossible to be ignored if the globs match as an unignore.

image

However, I put up a different take I was thinking about that might be better for the per config case. It might be a little more/less of a brain twister. In the new code, we disconnect the pushing of the config from the pushing of the globs. This new case will then return the unignored globs, but not push the config, which may be more useful for debugging or more clear as shown below (i.e. it does match technically)

Old approach:
image

Revised approach:
image

In terms of why cueing on the last item works. Globs are evaluated in order, so you can walk through the list of globs for a config. Given **/foo/special.js:
**/*.js. - File is ignored
!**/special.js. - File is unignored (this negates the previous matches)
**/foo/special.js - File is reignored (this negates the negation, or rather readds it from an unmatched state)

So using that logic, since we walk them in order, if the last matching glob is a positive matcher, than effectively all the other globs don't matter, it matches; the result is the file is ignored. If the last matching glob is an unignore (!), than no matter what the order of the things preceding it is, the final result is that it undoes all the ignores and it doesn't match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But won't there be the case of mixing of different types?

**/*.js
**/foo/*.js
!**/foo/special/*
**/foo/special/**/*.ts
...

I think we might better run a proper test for each glob with their priority?

Copy link
Author

@comp615 comp615 Dec 10, 2024

Choose a reason for hiding this comment

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

The globs we are cuing on have already been matched on line 69. So it's impossible to generate a file name that would cause both of these to match since they have mutually exclusive file extensions:
**/foo/*.js
**/foo/special/**/*.ts

Don't get me wrong, this is still not totally accurate with how ESLint actually works. For instance:

The pattern directory/** ignores the entire directory and its contents, so traversal will skip over the directory completely and you cannot unignore anything inside.

So it's very easy to make something that minimatch in the inspector will match but isn't in line with how ESLint works. But the issue is that we're fundamentally reimplementing the logic inside ESLint, so it will never be perfectly in sync and accurate. However, this change makes it more accurate.

Copy link
Author

Choose a reason for hiding this comment

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

@antfu I think we're on slightly different schedules, but let me know if you want to hop on discord or something and take this off async delay!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still not sure if I understand the logic here. Could we have some test cases to at least guard the behavior here?

Copy link
Author

Choose a reason for hiding this comment

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

Done! There was no testing in this package previously so I also had to setup and configure all that, so it's expanded the PR a bit. I used Mocha and followed the general style from other ESLint packages so that hopefully it's familiar/consistent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support !file/pattern/ ignores in the file matcher
2 participants