Skip to content
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

Creating custom sink nodes #384

Closed
roveo opened this issue Nov 11, 2020 · 2 comments · Fixed by #386
Closed

Creating custom sink nodes #384

roveo opened this issue Nov 11, 2020 · 2 comments · Fixed by #386

Comments

@roveo
Copy link
Contributor

roveo commented Nov 11, 2020

I tried to create a sink for my example plugin (to manually test #380) and encountered some issues.

It's not entirely clear from the docs how to do this. There are classes for Stream and Source that one can inherit from, but there is no such class for sinks. At the same time, sinks clearly demonstrate some special behavior. They have to add themselves to _global_sinks, without which they don't work (btw, why?). Since we're talking about plugins, I think this detail should be hidden from whoever wants to create one.

There is only one reference implementation of a sink (the sink) in the codebase. There's also sink_to_list, which is just a method of Stream, and sink_to_file, which is not really a sink — it's a function and it's not in Streams's dir().

You can really get by with just the built-in sink by passing a callable object to it, but @martindurant mentioned sinks in the plugin discussion, so there should be at least some situation where you'd want to create another.

@martindurant
Copy link
Member

Completely agree again! There should be a Sink class, which subclasses Stream, and makes sure _global_sink happens.

In case you are wondering, the reason they are different is, that a sink terminates a stream graph, and so there's nothing to be done with the node anymore: stream_node.mysink(). This means there is no reason to keep a reference to the final sink node, and so you can easily have your entire graph garbage collected. It may no be the best way to ensure that sinks stay alive.

Agree that sink is basically really a map_sink. We should be careful about renaming it, though.

@roveo
Copy link
Contributor Author

roveo commented Nov 12, 2020

No, I think sink is a good name, it's really the most generic thing you can do: sink to a function. And of course, backwards-compatibility and all that.

So, should I make a separate PR for this, or just do it together with plugins? Seems like a different thing, but probably not worth refactoring without having a plugin system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants