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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add more general support for negated exclude rules #58

Merged
merged 2 commits into from May 21, 2017

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented May 14, 2017

@davewasmer how does this look? very much appreciate the failing tests 馃憤

@bcoe bcoe requested a review from JaKXz May 14, 2017 19:35
@coveralls
Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage increased (+0.02%) to 89.07% when pulling 5e838f3 on add-failing-test-negated-exclude into 776d2a3 on master.

@davewasmer
Copy link
Collaborator

Seems good! I think this addresses the issues I raised.

One quick question - this logic seems somewhat complex (lots of double negatives), and it seems like the micromatch library supports all this logic directly via negated patterns. Why not just have a single array of include glob patterns, and if you want to exclude files, you use a negated glob? Then we could remove all this logic and just pass that array into micromatch. Is there a use case I'm missing here perhaps?

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

@davewasmer @bcoe I tried taking a different crack at this briefly over the weekend but unfortunately to support all the use cases we want, I ended up with something similarly complex...

I am ok with merging this, but I too am not a fan TBPH.

@bcoe
Copy link
Member Author

bcoe commented May 15, 2017

@davewasmer I can't quite figure out how to support this elegantly with the micromatch.all functionality -- unless I'm missing something.

The problem is that if you have an include pattern like foo/**, and a negated match like !foo/bar.js, if you have the file foo/bar.js, .any returns true because the rule matches foo/** (even though it fails !foo/bar.js) -- tldr; I couldn't off the top of my head figure out an approach that works elegantly, without building a few different lists of globs that we need to check in an appropriate order.

@bcoe
Copy link
Member Author

bcoe commented May 15, 2017

@davewasmer might be worth having an extra set of eyes on the micromatch API, to see if anything jumps out at you.

@davewasmer
Copy link
Collaborator

I think if you don't use micromatch.any, but instead use the default functionality, it might work:

> mm('foo/bar.js', [ 'foo/**', '!foo/**' ])
[]

@bcoe
Copy link
Member Author

bcoe commented May 15, 2017

@davewasmer want to submit a patch to this branch 馃槃 just added you to the project.

@davewasmer
Copy link
Collaborator

davewasmer commented May 16, 2017

Nothing like trying to implement it to help you find the flaws.

The critical shortcoming of the micromatch API is that if a filepath matches any negation, it's excluded no matter what. This means that it's impossible to do an "exclude everything except" type of pattern:

> mm('node_modules/foo.js', [ '!node_modules/**', 'node_modules/foo.js' ])
[]

Which is actually the exact use case that led me to find this issue in the first place.

Nevermind then 馃槈

@bcoe
Copy link
Member Author

bcoe commented May 16, 2017

@davewasmer sounds like we need to stick with my wonky logic for now?

@bcoe bcoe merged commit 08445db into master May 21, 2017
@bcoe bcoe deleted the add-failing-test-negated-exclude branch May 21, 2017 22:46
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.

None yet

4 participants