FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add Stream node constructor for sub-classing #442 by florentbr · Pull Request #445 · python-streamz/streamz · GitHub

/ streamz Public

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

Open
florentbr wants to merge 4 commits into
python-streamz:masterfrom
florentbr:subclassing
ConversationCommits (4)ChecksFiles changed
Open

Add Stream node constructor for sub-classing #442#445
florentbr wants to merge 4 commits into
python-streamz:masterfrom
florentbr:subclassing

Conversation

florentbr commented Dec 10, 2021

Copy link
Copy Markdown

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 reviewed Dec 10, 2021
View reviewed changes
Comment thread streamz/tests/test_core.py

martindurant commented Dec 13, 2021

Copy link
Copy Markdown
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 commented Dec 13, 2021

Copy link
Copy Markdown
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 commented Dec 13, 2021

Copy link
Copy Markdown
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 commented Dec 13, 2021

Copy link
Copy Markdown
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 commented Dec 13, 2021

Copy link
Copy Markdown
Member

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

martindurant reviewed Dec 15, 2021
View reviewed changes

martindurant left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

florentbr commented Dec 17, 2021

Copy link
Copy Markdown
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 commented Dec 20, 2021

Copy link
Copy Markdown
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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

martindurant martindurant left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL