Closed avegancafe closed 7 years ago
@phated not sure if there's rules/standards around writing tests, but here are some preliminary ones! Sorry for the long wait... was traveling a bunch so hadn't had time to write them, but I'd love some feedback! Pretty new to contributing to open source stuff so any criticism is greatly appreciated.
I just had a chance to do a cursory overview (didn't want to leave it for too long) and they are looking good. I like how you are using the stream utilities now. I'll do a full review coming up soon; sorry for the delay.
LGTM, but didn't we get rid of should
in https://github.com/gulpjs/glob-stream/commit/5a89acc20279b18bf4d48e86f8b90ad72e957143 ? Might want to make sure this was done on top of the latest tests.
No worries @phated! Busy time of year, whenever you have a chance.
@contra Not sure, I was basing this off of branch just rebased this on top of master, have a ref to the old one though if you guys think we should do that in a different PR.readable-stream
. Think we should rebase that as well and then I can update this one to match that?
I don't think we should have it rebased on master. The commits seem wrong now.
@kyleholzinger I looked at this deeper and I don't think we should have it rebased right now. Can you back those out?
Sure thing @phated! Just pushed the old stuff back
Thanks for the comments @phated! Just working on writing a test for a readable stream that's not first in the pipe, hopefully should be done soon
@kyleholzinger Thanks! Much appreciated 😃
Sweet! Thanks for checking it out @phated ! 😃
Some tests for #85