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

Add Stream node constructor for sub-classing #442 #445

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

florentbr
Copy link

This PR provides a constructor on the Stream which can be overridden for sub-classing the nodes.
This is to create or override methods without altering the original Stream class.
It gives a single point of entry to extend any node while chaining.
The behaviour remains the same unless Stream._new_node is overridden.

This is a usage example to subclass and inherit a Stream :

import streamz as sz

class MyStream(sz.Stream):
    def _new_node(self, cls, args, kwargs):
        if not issubclass(cls, MyStream):
            cls = type(cls.__name__, (cls, MyStream), dict(cls.__dict__))
        return cls(*args, **kwargs)

@MyStream.register_api()
class foo(sz.sinks.sink):
    pass

stream = MyStream()
stream.map(lambda x: x + 1).foo(print)
stream.emit(100)

@martindurant
Copy link
Member

Please ping me if you manage to get tests to pass. In the meantime, please note the warning suggesting to add pytest-asyncio to the CI env.

@florentbr
Copy link
Author

All the tests were successful on my fork:
https://github.com/florentbr/streamz/actions
Not sure what you expect me to do about the warning since I don't see how it relates to this PR.

@martindurant
Copy link
Member

what you expect me to do about the warning

This might be a convenient time to add the dep, as a service to the repo

@florentbr
Copy link
Author

I don't understand what "add pytest-asyncio to the CI env." and "add the dep, as a service to the repo" means and implies.
Could you please be more specific?

@martindurant
Copy link
Member

Never mind, I'll deal with it when I can

Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add some prose in the documentation about how to use this new feature?

@florentbr
Copy link
Author

It's not really a new feature. This change just exposes the constructor for the nodes which was otherwise inaccessible. It's not meant for a general usage, but for developers who wish to extend/tweak this library. I've already documented the API with a usage example. So I don't know what kind of prose you are expecting since it relates to basic OOP which is well documented on the web.

@martindurant
Copy link
Member

It's not really a new feature.

You are demonstrating usage that was previously not possible - adding methods to a subclass but not to the classes of this library. I think it's worth demonstrating that this is possible!

I have not yet managed to figure out why the kafka tests are sometimes failing here.

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 this pull request may close these issues.

2 participants