-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added initial draft for stream interfaces #55
Conversation
|
||
### Sink | ||
|
||
- `undefined .write(Stream stream)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .write
method could be confusing. It's more like a reverse .pipe
, but that method name should be reserved, because some classes may implemented full featured streams. Another proposed method name was .consume
.
It's the same for .read
. Reading triples from a source could be also described as writing all triples from the source to a stream. .filter
could be used, but would be confusing if the filter feature isn't used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the last meeting we collected the following alternative methods names. What do you think about them? Other proposals?
.accept
.consume
.input
.import
.append
.readFrom
.pull(From)
.reversePipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for import
/readFrom
-1 for pull
(without From
)/reversePipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 .consume / .import
-1 .accept / .input / .readFrom / .pull / .pullFrom / .reversePipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 .consume / .accept / .readFrom / .reversePipe / .append
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly recommend cribbing from:
https://nodejs.org/api/stream.html#stream_event_readable
https://streams.spec.whatwg.org/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlongley We're indeed using a subset of Node.js streams (intentionally not the full interface, because its backpressure control makes it expensive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have at least 2 * +1 for .import
. I will change my PR. If there are good arguments for a different method name, we can change that later.
IMO this PR received enough feedback to merge it after @bergos turns this feedback into new commits. After that everyone still have possibility to make new PRs proposing specific improvement to this initial draft, this should also allow for more granular changes and focused conversations. |
No description provided.