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

Support negated ignore patterns #70

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

webpro
Copy link

@webpro webpro commented Oct 30, 2024

Fixes #32

This PR adds support for negated ignore patterns. Please see #32 for more details and use cases.

This is a solution that should not impact performance at all if no negated ignore patterns are used. However, it does require a patched picomatch, and I'm not sure whether there's interest in a PR upstream. But before I'm even trying to go there I wanted to gauge interest here.

The idea/patch: we could leverage picomatch's onIgnore function to un-ignore if ignored matches also match a negated ignore pattern.

The problem: it requires to un-ignore matches within picomatch and this is a relatively low-impact solution for picomatch, but I'm afraid even requiring an explicit true (or false?) return value might still be a breaking change looking at the scale it's being used. Yet at the same time it could also actually mean a fix in certain cases and removing a major hurdle when looking to migrate from globby to tinyglobby.

This PR still needs some work:

  • Should we pass result.output to unignoreMatcher? I also saw result.input and I'm not sure about the difference.
  • Another thing that needs work: the options to pass to picomatch when creating unignoreMatcher.

Copy link

pkg-pr-new bot commented Oct 30, 2024

Open in Stackblitz

npm i https://pkg.pr.new/tinyglobby@70

commit: 6d8fcbd

@SuperchupuDev
Copy link
Owner

oh, wow. first of all, thanks a lot for taking your time into implementing this, such a nice solution. how would patching picomatch work for users who download tinyglobby from npm? sadly, i don't think patching it is viable long term, but we can figure out something else or send a PR to picomatch

@webpro
Copy link
Author

webpro commented Oct 30, 2024

how would patching picomatch work for users who download tinyglobby from npm? sadly, i don't think patching it is viable long term

Oh no, bad idea indeed, this wasn't my intention at all

but we can figure out something else or send a PR to picomatch

Yes, this PR is mostly to keep the conversation going. And for you and others the ability to actually try it out e.g. when looking to migrate away from globby and see if this is a viable route.

Would you be interested in this solution given that picomatch would accept my PR? Tbh I don't fancy publishing (and maintaining) a fork of picomatch, so I'm not sure what other options we have here.

@SuperchupuDev
Copy link
Owner

i think i'm fine with anything that doesn't involve patching, whether it is a picomatch PR or something more complex

@webpro
Copy link
Author

webpro commented Oct 30, 2024

Sorry I phrased that poorly. With "this solution" I meant this PR without the patch part. Thanks! WIP.

@SuperchupuDev
Copy link
Owner

i think so? i think i can see some things that could be refactored (unsure though) and probably needs a small cleanup when that happens but other than that yes

@webpro
Copy link
Author

webpro commented Oct 31, 2024

Here's the PR: micromatch/picomatch#137

@benmccann
Copy link
Contributor

When this is ready we should test it against cspell to make sure we don't break it since it uses a negated ignore pattern and the behavior here will be changing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for negated ignore patterns
3 participants