Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactor sinks #386
Refactor sinks #386
Changes from 6 commits
bceb79a
31018a7
7f04156
c001c36
a0c4c04
6ecd2c9
5c1b3a6
b51c6ae
9737377
269d3ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is fine, and I know it's only moved code, but would be good to not have to update these whenever the superclass might gain more keywords
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 don't know how we can both allow mixing stream-specific and func arguments in
**kwargs
and avoid explicitly listing them.I can see two solutions here:
func(*args, **kwargs)
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.
Please see here: 5c1b3a6#diff-eb662403bf433f3884021435c3f6cd010b387b4174a01367ae415b3e593cde5eR56-R60
Still looks ugly to me, but it does what you asked for :)
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.
What do you think, @CJ-Wright , is this better or worse?
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.
Integrate with
fsspec.open
?Why set the buffering?
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.
Used line buffering in development to see what's going on without delay, forgot to remove. Also some tests won't work as is without it, but I guess I'll just use
wait_for
.This is really a question about what filesystem functionality should be provided in core. If we want to integrate this sink with fsspec, then from_textfile should be updated too (or some other nodes added, like
from_fs
/sink_to_fs
), which is beyond sink refactoring. And then, we should think about how users might have a way to install fsspec extras (s3, hdfs etc.) and what other fs-related nodes might be useful, like:split
command)What I'm getting at is that it looks like a plugin to me. We could have
streamz[fs]
for nodes andfsspec
built-ins and thenstreamz[s3,hdfs]
for extras.