search

gulpjs / glob-stream

Readable streamx interface over anymatch.
MIT License
178 stars 51 forks source link

Implementing node-glob's `ignore` option, fixes #24 #40

Closed UltCombo closed 8 years ago

UltCombo commented 9 years ago

Quite a few tests are broken on my machine, not sure if I'm missing something or the ignore option is buggy in regard to the dot option and absolute paths (on Windows).

Fixes #24

yocontra commented 9 years ago

Actually if it cleans up the code lets go ahead and remove regexp support, I've never seen anyone use it. No need to keep that feature - it isn't even documented well afaik

UltCombo commented 9 years ago

Something odd is that Mocha was seemingly "cascading" certain test fails -- e.g. in my first commit above, the "glob-stream create() should handle RegExps as negative matchers" test shows up as a fail, but it passes when run in isolation.

UltCombo commented 9 years ago

I've removed RegExp support, but no change in the failed tests. I wonder if it is an issue with glob's ignore option, or if I'm just too sleepy to see an obvious error.

UltCombo commented 9 years ago

Oh -- I'm forgetting to unrelative the negative globs, doh.

UltCombo commented 9 years ago

Looks like there is still an issue with unrelative logic.

UltCombo commented 9 years ago

Oh wait, the unrelative logic is fine -- node-glob's ignore option seems to be screwing it up independent of using relative or absolute paths.

UltCombo commented 9 years ago

"should return a file name stream with dotfiles negated" is failing due to a node-glob issue.

UltCombo commented 9 years ago

I thought the error could be due to passing absolute paths (which use \ as dir separator in Windows) to the ignore option (according to the docs, node-glob always interprets \ as escape sequences).

Minimatch@2 does treat \ as a valid dir separator, but node-glob currently does not.

The test is still failing on Travis (Linux), so there must be yet another error...

yocontra commented 9 years ago

Any progress on this?

UltCombo commented 9 years ago

@contra there are some blocking issues:

UltCombo commented 9 years ago

@contra If you don't mind the breaking change regarding the dot behavior, we can skip those tests temporarily and I can try fixing the remaining logic again.

yocontra commented 9 years ago

@UltCombo Seems like the resolution from the glob issue was the ball is in your court. Do you need any help with this?

https://github.com/isaacs/node-glob/pull/166#issuecomment-76292453

UltCombo commented 9 years ago

@contra Well yes, I have near to no experience with the node-glob code base, and a good part of it seems nearly unreadable to me.

thirdcreed commented 8 years ago

I nearly have a working PR for this, there's one test broken: 'should return a file name stream with dotfiles negated' and it's timing out at 2000ms.

Any thoughts on what that might be, for some reason the stream is not emitting anything. https://github.com/thirdcreed/glob-stream/blob/master/index.js

thirdcreed commented 8 years ago

https://github.com/isaacs/node-glob/issues/232 This is a block for me. I think there's some bigger problem involving globs that end with "/**", a completely different matcher is used for those cases. There's some kind of race condition. It emits before the on end event is registered.

thirdcreed commented 8 years ago

I'm going to wait until issacs has time to answer it, I guess this above my head. Apparently the race condition thing is understood, It looks like its on the user to guarantee their events. Unless issacs has some insight on why their would be a race condition on the this one end event, in this one edge case. I’m out of ideas. I've been hacking on my own node-glob, but I can't seem to get that one fucking test to run.

It looks like @UltCombo's on this again anyway.

UltCombo commented 8 years ago

There are 23 failing tests (on both current master and @thirdcreed's branch) when testing on Windows. This does not make it easy to work on a fix and I'm relatively short on free time.

I guess I can give this another shot on my Linux or Mac box soon.

@contra May I suggest adding AppVeyor (Windows CI)? Here's a sample appveyor.yml just in case.

yocontra commented 8 years ago

@UltCombo We use appveyor on some other stuff, feel free to add it if you need it

UltCombo commented 8 years ago

@contra Do you have an appveyor config of your choice to add to this repository? Or should I just create one using the same Node.js versions as this repo's Travis config?

phated commented 8 years ago

https://github.com/gulpjs/gulp-cli/blob/master/appveyor.yml

jonschlinkert commented 8 years ago

Something odd is that Mocha was seemingly "cascading" certain test fails

if there are multiple tests with the same description, mocha will run them all, even if one has .only on it. I remember this driving me crazy a few times before @doowb noticed what was happening and pointed it out. again, not sure if it's related. but if not, maybe it will save someone else from going crazy lol.

phated commented 8 years ago

Closing in favor of #71 which is passing all the tests. Thanks for putting in a lot of the work here @UltCombo

UltCombo commented 8 years ago

Oh, this was still on my TODO list, but kept falling off the radar.

No problem, and indeed, #71 seems more suitable and up-to-date. I'll comment there if I find anything odd.