-
Notifications
You must be signed in to change notification settings - Fork 56
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
[WIP] Issue198 #209
[WIP] Issue198 #209
Conversation
Current coverage is 77.97% (diff: 100%)@@ master #209 diff @@
==========================================
Files 80 80
Lines 6637 6657 +20
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 5115 5191 +76
+ Misses 1522 1466 -56
Partials 0 0
|
@@ -948,7 +959,7 @@ def run(self, socket=None): | |||
self.fire(started(self)) | |||
|
|||
try: | |||
while self.running or self._queue: | |||
while self.running or len(self): |
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 think we used this explicit check because if a component would overwrite __len__
this would have implications. See #51 for a similar issue.
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.
Is it worth abstracting out the internal queue into a class that does what we want here? We could make things much more clear then right?
class EventQueue(object):
def __init__(...):
self._queue = deque()
self._priority_queue = []
@@ -219,7 +221,7 @@ def __repr__(self): | |||
|
|||
channel = "/{0:s}".format(str(getattr(self, "channel", ""))) | |||
|
|||
q = len(self._queue) | |||
q = len(self) |
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.
For the same reason we should be explicit here in case someone overrides __len__
on their components :/ People do silly things :)
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.
Yes, you two are absolutely right -- a custom object is probably the "most compatible"
Let's also get @spaceone view on this? |
@prologic I had a look at the changes which from the diff look okay. But I need to dig further into this. |
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.
LGTM. Just some small not so important questions.
@@ -126,6 +127,41 @@ def __init__(self, timeout): | |||
self.tick_handler = None | |||
|
|||
|
|||
class EventQueue(object): |
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.
Maybe private _EventQueue?
self._flush_batch = 0 | ||
|
||
def __len__(self): | ||
return len(self._queue) + len(self._priority_queue) |
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.
Maybe implement also __nonzero__
/__bool__
?
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.
Documentation for __nonzero__
/__bool__
:
When this method is not defined,
__len__()
is called, if it is defined, and the object is considered true if its result is nonzero.
def __len__(self): | ||
return len(self._queue) + len(self._priority_queue) | ||
|
||
def drainFrom(self, other_queue): |
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.
Do we want to keep the camelCase thing for new implementations? PEP8 wants lower_underscoe.
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 am fine with either, a mixed codebase looks somewhat weird though :D
@@ -126,6 +127,41 @@ def __init__(self, timeout): | |||
self.tick_handler = None | |||
|
|||
|
|||
class EventQueue(object): |
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.
Add __slots__ = ('_queue', '_priority_queue', '_counter', '_flush_batch')
?
|
||
def dispatchEvents(self, dispatcher): | ||
if self._flush_batch == 0: | ||
# XXX: Might be faster to use heapify instead of pop + |
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 personally like s/XXX/FIXME/ more.
@@ -909,7 +930,7 @@ def tick(self, timeout=-1): | |||
if self._running: | |||
self.fire(generate_events(self._lock, timeout), "*") | |||
|
|||
self._queue and self.flush() | |||
len(self._queue) and self.flush() |
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.
Maybe split into 2 lines? Why should we use this syntax? Performance?
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.
No reason imo, splitting it makes the intent certainly cleaner
If this works out on PyPY the patch needs cleanups. I am also imagining
the following: Always use a deque and add a flag to the manager if
priority events should be supported. If not we don't have to use the
heap queue at all which might result in performance improvements.