Skip to content

Conversation

@kyleknap
Copy link
Contributor

Abstraction is a thread that processes results in the result queue by
passing each result from the queue to the ResultRecorder and ResultPrinter.

cc @jamesls @JordonPhillips

Abstraction is a thread that processes results in the result queue by
passing each result from the queue to the ResultRecorder and ResultPrinter.
@kyleknap kyleknap added the pr:needs-review This PR needs a review from a Member. label Aug 18, 2016
except queue.Empty:
pass

def _process_result(self, result):
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth just having a generic ResultHandler interface and having the __init__ take a list of result handlers? You could then either write adapters for the recorder/printer or update them to have the same interface.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Posssibly. I was actually considering doing that. The only reason I did not was because I did not see needing it in doing the port, but taking an arbitrary list of handlers is pretty flexible so I would be totally fine with making that the interface.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, don't feel too strongly about it for this specific PR, it would just clean up the code a little bit:

for handler in self._result_handlers:
    handler.handle_result(result)

vs. having to check if a particular handler was None or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm updating it. I did not like the None check either.

It now accepts a list of handlers to call when processing through the
result queue.
@jamesls
Copy link
Member

jamesls commented Aug 18, 2016

:shipit:

I assume we're going to just change the existing recorder/printer to just have a __call__(self, result):?

@kyleknap
Copy link
Contributor Author

@jamesls I realized that as well when I was looking over the first commit I pushed. You just beat me to it. I went a head and changed the interface to use __call__() instead. So it would be good to get a look at the last commit I pushed.

@jamesls
Copy link
Member

jamesls commented Aug 19, 2016

:shipit:

@kyleknap kyleknap merged commit 879774d into aws:integrate-s3transfer Aug 19, 2016
@kyleknap kyleknap deleted the result-processor-v2 branch August 19, 2016 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:needs-review This PR needs a review from a Member.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants