FIX: connect doesn't break zip now #248
Conversation
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
=======================================
Coverage 94.96% 94.96%
=======================================
Files 13 13
Lines 1609 1609
=======================================
Hits 1528 1528
Misses 81 81Continue to review full report at Codecov.
|
|
Ready for review! |
| if downstream: | ||
| downstream._inform_asynchronous(asynchronous) | ||
|
|
||
| def _add_upstream(self, upstream): |
There was a problem hiding this comment.
Although this is internal API, I think one-line docstrings would be useful for developers.
Also, I suspect that the old way (self.upstream.downstreams.add/remove) is used in many places - do you think they ought to be changed to use this formalism now?
There was a problem hiding this comment.
I think the classes which use the old way should be migrated, otherwise they might run into issues with zip and such. I'll look to see if that pattern is used elsewhere.
There was a problem hiding this comment.
The new slice method has it, sorry: https://github.com/python-streamz/streamz/blob/master/streamz/core.py#L795
There was a problem hiding this comment.
Would appreciate if you merged from master and made sure that slice works
There was a problem hiding this comment.
sorry about that, forgot it was there!
There was a problem hiding this comment.
I think this is taken care of now? https://github.com/python-streamz/streamz/pull/248/files#diff-d4e512993f47710ea9e7155d66011a24R813
There was a problem hiding this comment.
Oh I am sorry - I don't know why I didn't see your commits come in.
streamz/core.py
Outdated
|
|
||
| def _add_upstream(self, upstream): | ||
| if self.upstreams == [None]: | ||
| self.upstreams = [upstream] |
There was a problem hiding this comment.
Any reason to make a new list, instead of self.upstreams[0] = upstream?
I suppose you were just copying how the code looked previously.
streamz/core.py
Outdated
| if len(self.upstreams) == 1: | ||
| self.upstreams = [None] | ||
| else: | ||
| self.upstreams.pop(self.upstreams.index(upstream)) |
There was a problem hiding this comment.
self.upstreams.remove(upstream)?
streamz/core.py
Outdated
|
|
||
| def _remove_upstream(self, upstream): | ||
| if len(self.upstreams) == 1: | ||
| self.upstreams = [None] |
streamz/core.py
Outdated
|
|
||
| def _remove_upstream(self, upstream): | ||
| if self.emit_on == upstream: | ||
| raise RuntimeError("Can't remove the emit on upstream, consider" |
There was a problem hiding this comment.
Can you make this message clearer?
streamz/core.py
Outdated
| def _add_upstream(self, upstream): | ||
| self.last.append(None) | ||
| self.missing.update([upstream]) | ||
| if self.emit_on != self.upstreams: |
There was a problem hiding this comment.
This equality check seems a little odd - can't we just store whether emit_on was originally given as None? Same comment for _remove_upstream.
|
@martindurant since |
3 participants
No description provided.