balena-io-modules / blockmap

Tizen's block map format
Apache License 2.0
6 stars 0 forks source link

Make Streams API mirror node core's API more closely #16

Closed jhermsmeier closed 7 years ago

jhermsmeier commented 7 years ago

Connects to: #15

Changes:

lurch commented 7 years ago

Perhaps it's just me misunderstanding things, but in some places fd is specified as a Number, but in other places it's specified as a String? :confused:

jhermsmeier commented 7 years ago

Nope, not just you – I mangled the type. Fixed now.

lurch commented 7 years ago

LGTM, but I don't know the nodejs API well enough to give this "official approval" ;-)

jhermsmeier commented 7 years ago

Now that I think about it, I should probably implement the other fs.ReadStream options as well, to be fully compatible with that API.

jhermsmeier commented 7 years ago

I've added the other fs.ReadStream options (start, end, autoClose) as well now, with exception of mode, as that's only used when creating a file, which is not applicable in this case.

lurch commented 7 years ago

Should createReadStream be documented as having the same options as new ReadStream?

jhermsmeier commented 7 years ago

Should createReadStream be documented as having the same options as new ReadStream?

Yup, fixed.

lurch commented 7 years ago

One more question... ;-) If we pass in a filename instead of an fd, does the autoClose setting effectively get ignored? Does that need a unit-test?

jhermsmeier commented 7 years ago

No, the autoClose option is not coupled to the fd option, so if you pass a filename and autoClose: false, then the readStream.fd will still be open & usable after the stream has ended. Test added.

jviotti commented 7 years ago

I'd love to see the following test cases:

jhermsmeier commented 7 years ago

Good catch! Added tests, and fixed a case they caught.