-
Notifications
You must be signed in to change notification settings - Fork 3
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
Major refactor to allow lazy sync iterators. #10
Conversation
Fixes DelfinaCare#6. Because the code was getting too unweildy and hard to follow, I refactored the iterator proxies to their own classes in a separate file.
We yield only outside the lock to avoid potential deadlocks.
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.
More to review but this is the important stuff so far
Response to comment from DelfinaCare#10
We use an enum to thoroughly document the possible states the yielding function can be in, making the code a lot more readable IMHO!
ba26773
to
cf63c61
Compare
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.
Will read through the tests in detail on the next pass
once/_iterator_wrappers.py
Outdated
self.finished = True | ||
self.generator = None # Allow this to be GCed. | ||
self.generating = False | ||
else: |
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.
nit: It's hard to tell if this else
is on the if
or the try
. I think you could just in-line the else below result = self.generator.send(next_send)
, like:
try:
result = self.generator.send(next_send)
with self.lock:
self.results.append(result)
self.generating = False
except StopIteration:
...
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 this any clearer?
It is no longer set on the object, per discussion.
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.
Nice! Last round I think
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.
Woo we made it! Nice work!
Fixes #6.
Because the code was getting too unweildy and hard to follow, I refactored the iterator proxies to their own classes in a separate file.