state/api: Refactor client API watchers

state/api: Refactor client API watchers

Introduced commonWatcher and refactored
the EntityWatcher at API client-side to
help reduce code duplication.

In a following CL this will be used to
introduce 2 new watchers.
It's all part of implementing the API
calls needed by the provisioner.

https://code.launchpad.net/~dimitern/juju-core/042-api-client-common-watcher/+merge/165581

(do not edit description out of merge proposal)

[dimitern, 2013-05-24 11:47:11]

Please take a look.

[mue, 2013-05-24 11:57:02]

LGTM, only one comment.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode402
state/api/apiclient.go:402: // These fields must be set by the embedding
watcher.
So make them as argument of init() and check them there.

[rog, 2013-05-24 12:02:42]

LGTM with some documentation added/

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode403
state/api/apiclient.go:403: newResult func() interface{}
perhaps a comment on these fields would be good.

for example:

// newResult must return a pointer to a value
// of the type returned by the watcher's Next call.

and

// call should invoke the given API method¸
// placing the call's returned value in result.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode407
state/api/apiclient.go:407: func (w *commonWatcher) init() {
// init must be called to initialise an embedded commonWatcher's fields.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode411
state/api/apiclient.go:411: func (w *commonWatcher) commonLoop() {
// commonLoop implements the loop structure common
// to the client watchers. It should be started
// in a separate goroutine by any watcher that
// embeds commonWatcher. It kills the commonWatcher's
// tomb when an error occurs.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode524
state/api/apiclient.go:524: // Note: This particular watcher does not send any
i'm not sure this particular note is worth it - we already have a comment above,
and the type returned by Changes should make it clear.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode533
state/api/apiclient.go:533: func (w *EntityWatcher) Changes() <-chan struct{} {
// Changes returns a channel that receives a value
// when a given entity changes in some way.

[dimitern, 2013-05-24 12:20:00]

*** Submitted:

state/api: Refactor client API watchers

Introduced commonWatcher and refactored
the EntityWatcher at API client-side to
help reduce code duplication.

In a following CL this will be used to
introduce 2 new watchers.
It's all part of implementing the API
calls needed by the provisioner.

R=TheWorkingMue, rog
CC=
https://codereview.appspot.com/9667048

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode402
state/api/apiclient.go:402: // These fields must be set by the embedding
watcher.
On 2013/05/24 11:57:02, TheWorkingMue wrote:
> So make them as argument of init() and check them there.

I'll check they're set in init(), good point, but I don't think passing them as
argument is necessary.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode403
state/api/apiclient.go:403: newResult func() interface{}
On 2013/05/24 12:02:42, rog wrote:
> perhaps a comment on these fields would be good.
> 
> for example:
> 
> // newResult must return a pointer to a value
> // of the type returned by the watcher's Next call.
> 
> and
> 
> // call should invoke the given API method¸
> // placing the call's returned value in result.

Done.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode407
state/api/apiclient.go:407: func (w *commonWatcher) init() {
On 2013/05/24 12:02:42, rog wrote:
> // init must be called to initialise an embedded commonWatcher's fields.

Done.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode411
state/api/apiclient.go:411: func (w *commonWatcher) commonLoop() {
On 2013/05/24 12:02:42, rog wrote:
> // commonLoop implements the loop structure common
> // to the client watchers. It should be started
> // in a separate goroutine by any watcher that
> // embeds commonWatcher. It kills the commonWatcher's
> // tomb when an error occurs.

Done.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode524
state/api/apiclient.go:524: // Note: This particular watcher does not send any
On 2013/05/24 12:02:42, rog wrote:
> i'm not sure this particular note is worth it - we already have a comment
above,
> and the type returned by Changes should make it clear.

Removed.

https://codereview.appspot.com/9667048/diff/1/state/api/apiclient.go#newcode533
state/api/apiclient.go:533: func (w *EntityWatcher) Changes() <-chan struct{} {
On 2013/05/24 12:02:42, rog wrote:
> // Changes returns a channel that receives a value
> // when a given entity changes in some way.

Done.