Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Convert glob-stream to proper readable stream #85

Merged
merged 16 commits into from
Feb 13, 2017
Merged

Conversation

phated
Copy link
Member

@phated phated commented Dec 14, 2016

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
Copy link
Member Author

phated commented Dec 14, 2016

Implementation as per isaacs/node-glob#291

@avegancafe
Copy link

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
Copy link
Member Author

phated commented Dec 27, 2016

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
Copy link

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

@phated
Copy link
Member Author

phated commented Dec 30, 2016

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

@yocontra
Copy link
Member

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

@avegancafe
Copy link

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!

@avegancafe avegancafe mentioned this pull request Jan 7, 2017
@phated phated force-pushed the readable-stream branch 3 times, most recently from 4bb96b3 to 267aa39 Compare January 24, 2017 20:56
@phated
Copy link
Member Author

phated commented Jan 24, 2017

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 phated removed the help wanted label Feb 2, 2017
@phated phated changed the title WIP: Convert glob-stream to proper readable stream Convert glob-stream to proper readable stream Feb 2, 2017
@phated
Copy link
Member Author

phated commented Feb 2, 2017

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
Copy link
Member Author

phated commented Feb 3, 2017

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

@phated
Copy link
Member Author

phated commented Feb 3, 2017

Also added @contra and @terinjokes as reviewers.

@phated phated requested review from terinjokes and yocontra February 3, 2017 00:58
Copy link
Member

@yocontra yocontra left a comment

Choose a reason for hiding this comment

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

Could use some more documentation on why there are two interfaces exported, otherwise looks good to me.

@phated
Copy link
Member Author

phated commented Feb 13, 2017

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

@phated phated merged commit b3d1c69 into master Feb 13, 2017
@phated phated deleted the readable-stream branch February 22, 2017 00:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants