Expose read and on('readable') #5

felixSchl · 2016-05-19T18:00:42Z

Sill WIP, but feedback welcome.

felixSchl · 2016-05-19T21:25:01Z

Are you guys open to using purescript-spec for the tests? I think we can improve the coverage and readability a lot by going down that route.

hdgarrood · 2016-05-19T22:43:02Z

src/Node/Stream.js

+exports.onReadable = function(s) {
+    return function(f) {
+        return function() {
+            s.on('readable', function() {


I think just s.on('readable', f) should work here, right?

hdgarrood · 2016-05-19T23:03:42Z

Sorry for the nitpicks, this looks good 👍 just checking, this doesn't break anything, right?

I'd rather not depend on purescript-spec, as if it or anything it depends on later decides to depend on this package, it turns into a bit of a mess, unfortunately.

hdgarrood · 2016-05-19T23:04:34Z

src/Node/Stream.purs

+data Chunk
+data Data = BufferData Buffer
+          | StringData String
+          | UnknownData Chunk


What do you think about using Foreign here?

Instead of defining a new type Chunk, I mean.

I don't mind, but it might make the code a bit harder to read because of the ambiguity of Foreign.

My impression is that object mode lets you put any javascript value through a stream (apart from null which has a special meaning for the end of the stream). Given this, I think Foreign is appropriate; that is what the type represents, and you also get plenty of useful functions for turning Foreign values into data types that you can do stuff with.

Why do you say Foreign is more ambiguous than your Chunk?

Looking at the node API again though, if we were going to support object mode, I think I would rather have an entirely separate data type for streams in object mode anyway, especially since no Node APIs use them. I don't want to have to worry about whether a stream is in object mode when it ought to be possible to rule that out at compile time.

hdgarrood · 2016-05-19T23:05:25Z

What about it is WIP?

felixSchl · 2016-05-19T23:16:11Z

Thank you for your valuable feedback. I checked it in before heading to work today so I knew there would be some rough edges (like e.g. forgetting to match Nothing), hence the WIP. I also think it's still WIP because the tests are not scrutinizing enough (I got away with a obviously missing pattern).

hdgarrood · 2016-05-19T23:43:53Z

Cool :)

Also, do you have a use case for the UnknownData constructor? It seems like the only case where we would need it would be if the stream was in object mode, which I think I'd rather not try to support until we have a real use case for that (just because, in my experience, trying to do bindings without a use case doesn't work very well). How about throwing an error in the case where the data is neither a String nor a Buffer?

felixSchl · 2016-05-19T23:47:56Z

Yeah I was thinking of objectMode, but I agree with what you are saying about guessing usecases.

felixSchl · 2016-05-20T01:03:06Z

Alright, I forced pushed my fixes. I have made all changes as per review and added another test. Lucky I did, it caught another problem. If you are happy with this version, I am happy to take away the "WIP" from the title and have it merged.

felixSchl · 2016-05-20T01:05:38Z

Actually now that we disregard object mode, we are back to two constructors, and could use Either instead of Data again.

hdgarrood · 2016-05-20T01:13:35Z

Great :)

I was just thinking the same thing about Data vs Either, yeah. I'd rather stick to one or the other; there doesn't seem to be much point in having a Data type if it's isomorphic to Either String Buffer and if we're going to only use Either String Buffer in the exported API. I don't feel strongly, but I think I'd vote for sticking to Either everywhere, just to avoid breakage.

felixSchl · 2016-05-20T01:32:36Z

I agree. I removed the ADT and used a type alias.

felixSchl · 2016-05-20T01:33:32Z

I see a problem though between onDataString and readString. onDataString takes a encoding parameter. Should readString do, too? Or should there be two variants?

hdgarrood · 2016-05-20T01:33:39Z

src/Node/Stream.js

+        return function(f) {
+            return function() {
+                r.on('data', function(data) {
+                    f(readChunk(data));


I think this is missing a () at the end of the line.

Good catch... bummer the tests did not pick up on it.

Did the tests catch this? If not, could you add one please?

Hmmm, actually not sure how to test this, since we are not using Aff or ContT. In other words, if no callback is called, Eff don't care.

Oh, good point. Ok, let's not worry then.

hdgarrood · 2016-05-20T01:41:32Z

I'd like read to follow the same pattern as onData, ideally, yeah. My reasoning was that this library should discourage calling setEncoding on the stream, because doing so loses information and irreversibly changes the behaviour of all streaming functions, just for the sake of a minor convenience. Therefore it tries to be most comfortable when setEncoding has not been called. onData is for if you want a Buffer, and onDataString is for if you want a String (and because the stream is actually producing Buffer values, you need an encoding to get String values out). onDataEither is for the (hopefully very rare) case where you have a stream which might have had setEncoding called on it, and you need to handle that. I think ideally we'd make read and readString follow the same pattern, if that makes sense.

hdgarrood · 2016-05-20T01:48:56Z

src/Node/Stream.purs

+onDataEither :: forall r eff. Readable r (err :: EXCEPTION | eff) -> (Either String Buffer -> Eff (err :: EXCEPTION | eff) Unit) -> Eff (err :: EXCEPTION | eff) Unit
+onDataEither r cb = onData' r cb
+
+-- | Listen for `data` events, returning 


Maybe just inline this definition into onDataEither now?

felixSchl · 2016-05-20T16:40:38Z

Alright so readString takes an Encoding now and throws if it reads a string from the buffer because that would imply setEncoding was called, just like onDataString. I have also inlined onDataEither.

hdgarrood · 2016-05-20T16:56:58Z

src/Node/Stream.purs

 -- | A duplex (readable _and_ writable stream)
 type Duplex = Stream (read :: Read, write :: Write)

+type Data = Either String Buffer


What does this type synonym (and also ChunkReader) give us over writing out the real types? I generally tend to avoid type synonyms like these, as then there's more to keep in your head when you're reading the code.

hdgarrood · 2016-05-20T17:03:01Z

src/Node/Stream.purs

+  case v of
+       Nothing          -> pure Nothing
+       Just (Left _)    -> throw "Stream encoding should not be set"
+       Just (Right buf) -> pure <$> (unsafeInterleaveEff $ Buffer.toString enc buf)


How would you feel about using Just rather than pure here?

felixSchl · 2016-05-20T17:14:55Z

Alright, I'll do proper commits now until we're done. Then I'll squash and then we can merge

felixSchl · 2016-05-20T17:15:24Z

Reason being it's easier for us to review how we got here.

hdgarrood · 2016-05-20T17:23:23Z

Ok, absolute last thing and then we're done - the comment on readChunk refers to the Data type which no longer exists. You could update it or remove it, up to you.

hdgarrood · 2016-05-20T17:23:58Z

Also, if you are going to just update it, I think it ought to go on the function itself, rather than the Chunk data type (as it currently is)

felixSchl · 2016-05-20T17:25:01Z

Sweet, I just removed it. It's also squashed.

hdgarrood · 2016-05-20T17:27:17Z

Looks great, thanks very much! Sorry this was such an ordeal, it's just that it's quite high up in the dependency tree of purescript-node packages so I think it pays to be extra careful. 🎉

felixSchl · 2016-05-20T17:28:22Z

No worries, I enjoyed working through this and learned a thing or two.

hdgarrood reviewed May 19, 2016
View reviewed changes

felixSchl force-pushed the feature/read branch from 5bf0052 to a04c1e8 Compare May 20, 2016 01:00

felixSchl force-pushed the feature/read branch from a04c1e8 to d26387c Compare May 20, 2016 01:03

felixSchl force-pushed the feature/read branch from d26387c to b5bc23c Compare May 20, 2016 01:31

hdgarrood reviewed May 20, 2016
View reviewed changes

felixSchl force-pushed the feature/read branch from b5bc23c to f9c1687 Compare May 20, 2016 01:39

hdgarrood reviewed May 20, 2016
View reviewed changes

felixSchl force-pushed the feature/read branch from f9c1687 to 52f7d24 Compare May 20, 2016 16:39

felixSchl changed the title ~~[WIP] Expose read and on('readable')~~ Expose read and on('readable') May 20, 2016

hdgarrood reviewed May 20, 2016
View reviewed changes

Expose read and on('readable')

21d27d5

felixSchl force-pushed the feature/read branch from 40e02c4 to 21d27d5 Compare May 20, 2016 17:24

hdgarrood merged commit 8b9a6d5 into purescript-node:master May 20, 2016

Expose read and on('readable') #5

Expose read and on('readable') #5

Uh oh!

Conversation

felixSchl commented May 19, 2016

Uh oh!

felixSchl commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented May 19, 2016

Uh oh!

felixSchl commented May 19, 2016

Uh oh!

hdgarrood commented May 19, 2016

Uh oh!

felixSchl commented May 19, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

hdgarrood commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

hdgarrood commented May 20, 2016

Uh oh!

hdgarrood commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

hdgarrood commented May 20, 2016

Uh oh!

felixSchl commented May 20, 2016

Uh oh!

Uh oh!