search

gulpjs / glob-stream

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

Convert glob-stream to proper readable stream #85

Closed phated closed 7 years ago

phated commented 7 years ago

I think the implementation is pretty solid (except the few TODOs) but I really need some help with tests. I'd love for someone to take a stab at them.

phated commented 7 years ago

Implementation as per https://github.com/isaacs/node-glob/issues/291

avegancafe commented 7 years ago

Hey man! Sorry for the late start, but just getting to writing some tests now. Is there any description of what GlobStream's behavior should be? I'm pretty unfamiliar streams so not sure if that's innately obvious. I read over isaacs/node-glob#291 but that seemed more like a direct question more than a description of the overall behavior

Happy holidays!

phated commented 7 years ago

No worries and happy holidays.

The general description is that it should be a Readable stream (so nothing should be able to be pushed onto the stream, only read from it) that emits an object for each match. The best way for testing streams seems to be something like https://github.com/gulpjs/vinyl-fs/blob/master/test/src.js#L154-L157

Feel free to ask any other questions you have on this thread and I'll try to answer the best I can.

avegancafe commented 7 years ago

Sounds awesome! Thanks for the explanation, that definitely helps. I'll hop right on 'em 🏃

phated commented 7 years ago

Assigned this to @contra incase he has time to make suggestions/help @kyleholzinger writing tests this week

yocontra commented 7 years ago

@kyleholzinger LMK if you need any assistance w/ this.

avegancafe commented 7 years ago

Will do, thanks @contra @phated ! Sorry, been traveling for the holidays so haven't had much time to work but will jump on this as soon as I can!

phated commented 7 years ago

Merged the tests from @kyleholzinger with a bit of cleanup and rebased against the test-refactor branch (that one should be merged in way before this).

There's still more tests to write to get the coverage back up to 100% and TODOs inline that need to be finished. If anyone wants to jump on them, feel free.

phated commented 7 years ago

Pending tests running and #88 being reviewed, I think this is done. I'm probably going to squash this to 1 commit and label it "Breaking"

phated commented 7 years ago

@shinnn would you be available to help review this? I remember you submitted a PR to make us Streams3 compliant in the past.

phated commented 7 years ago

Also added @contra and @terinjokes as reviewers.

phated commented 7 years ago

I guess it's time to merge this. I was hoping for some more reviews but oh well.