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

Bug fix for #96 (.not returning incorrect results on windows) #97

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

doowb
Copy link
Member

@doowb doowb commented Jul 3, 2017

Bug Fix

  • All existing unit tests are still passing (if applicable)
  • Add new passing unit tests to cover the code introduced by your pr
  • Update the readme (see readme advice)
  • Update or add any necessary API documentation
  • Add your info to the contributors array in package.json!

Description

Currently, the matches returned from micromatch inside the .not method are removed from the list array (which is the original list of files passed into micromatch.not.

This PR uses utils.unixify on the list array to ensure that the original files are unixified (if specified in the options) so when "diffing" the correct file paths will be remove and returned.

Updates made

 - update .not method in the test support matcher
 - unixify the list of files passed into micromatch.not

Using utils.unixify on the list of files ensures that when the results come back, both lists
will be unixified if the unixify option is set to true.
@jonschlinkert
Copy link
Member

jonschlinkert commented Jul 3, 2017

@doowb thanks!

The changes look good, but ideally we should try to find a way to not unixify twice, since I believe that's already done for list (

var matches = utils.diff(list, micromatch(list, patterns, opts));
), everywhere but the first argument passed to diff()

edit: hmm what if we add a .unified boolean to the array. we can make it non-enumerable if necessary, but we can check that before unixifying in the .unixify utility to see if it's already done nvm, unifixy takes a string, not an array. But we could still do something similar

@doowb
Copy link
Member Author

doowb commented Jul 3, 2017

everywhere but the first argument passed to diff()

That's what's causing the issue, because the original paths in the first argument are not unixified, but the paths that are returned are.

But we could still do something similar

Could the unixify utility function keep a cache of file paths so it only unixifies it once? I think you had tried that out for speed improvements before, but I don't think unixify was used enough to see a benefit.

@jonschlinkert
Copy link
Member

That's what's causing the issue, because the original paths in the first argument are not unixified, but the paths that are returned are.

right. that's what I meant.

Could the unixify utility function keep a cache of file paths

I wouldn't do that, since we'd then need to implement an lru cache or something that would prevent memory leaks. let's just merge this since it fixes the issue, and I can take a look at optimizing after. thx

@doowb doowb merged commit 6ca8ba6 into master Jul 11, 2017
@doowb doowb deleted the unixify-not branch July 11, 2017 18:53
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.

basename: true and unixify: true breaks .not()
2 participants