fs,stream: introduce FileSystemStream#36408
Conversation
Adds a Trait to group functionality of fs.ReadStream and fs.WriteStream.
nodejs-github-bot
added
the
fs
|
ping @ronag |
ronag
left a comment
ronag
left a comment
There was a problem hiding this comment.
Nothing wrong with it per se but it looks to me like unnecessary abstraction...
|
Me too, my first impression was that it was useless bloat, but now when I see it fully implemented, I must admit that it is a PR that deletes more lines than it introduces - even if most of the difference comes from removing a few large comment blocks |
| // stream.closed = true; | ||
| cb(err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This doesn't need to live on the prototype. Just make it a function taking stream as first param.
| cb(er || err); | ||
| }); | ||
| stream.fd = null; | ||
| function FileSystemStream(path, options) { |
There was a problem hiding this comment.
This doesn't need to be a constructor function. Can be just a regular function taking stream as first param.
There was a problem hiding this comment.
But it's taking the options to, well construct the (Read|Write)Stream instance (defining properties on it)…
|
I tend to agree with Robert regarding this sort of abstraction not making the code easier - I didn't want to say that before to not influence bias but I am -0 on this (won't block but generally not in favour). |
|
I don't feel strongly about this myself, I just find the |
5 participants
Adds a Trait (abstract class?) to group functionality of
fs.ReadStreamandfs.WriteStream.Refs: #35922 (comment)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes/cc @mmomtchev