Code review comment for lp:~rvb/gwacl/poller

Thanks for the review.

>
> Could you document Poller to set the right expectations with the reader? The
> "flyweight" approach in particular looks a bit confusing. My understanding is
> that the Poller gives you a response and then you need to ask that same Poller
> whether that response means that you're done... I think I understand why it's
> done that way (it allows for an immutable poller) but it would help to have it
> stated.

Added a comments.

> I like the separation between the Poller interface and the internal
> implementation. Exposing the x509Response doesn't really fit in though. My
> recollection is that we had meant that to be internal to gwacl, given the
> duplication with http.Response, and now x509Response is no longer exported.
> This part needs fixing, I think — though not necessarily in the same branch.

You're right, now that x509Response is not exposed, the method in the poller interface should not be exposed either. Same for startPolling which is more of an internal utility method used by more concrete implementations such as StartOperationPolling.

> On reflection, maybe StartPolling() isn't quite the right name, in that it
> implies that it won't do the actual polling.

Hum, I don't really have a better idea right now so I'll land this as is and we will see if we come up with a better name later.

> Typo in the documentation for operationPoller: "A object."

Fixed.

> The documentation for operationPoller.Poll() and operationPoller.IsDone() gets
> lost in trying to describe the implementation. Document what they mean to the
> caller, not how they do their work! As a caller, I really don't care whether
> Poll() issues a GET request or a HEAD request — gwacl's exact purpose is to
> hide that sort of thing from me. What I care about as a caller is primarily:
> that I need to call these methods in order to wait for my prior request to
> complete and secondarily: that it makes outgoing requests and that it blocks.

This is really internal stuff, no user will ever call these methods directly, hence the focus on explaining what the methods do internally.

> In operationPoller.IsDone(), I think I see one of those xml.Unmarshal() calls
> that need an error check.

Indeed, I've left it as is, we will fix this in another branch.

> In TestStartOperationRetries, "InProgress" is misspelled as "InProgres" in a
> comment and "Succeeded" is misspelled twice as "Succeded" — which may be
> working just because it's consistent, but it's late for me.

oops, fixed.

Thanks again for the review.