Code review comment for lp://staging/~gary/juju-core/megawatcher_api

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM modulo the suggestions below. Nice simplification with the
clientError() stuff!

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

https://codereview.appspot.com/7563046/diff/1/state/api/apiclient.go#newcode131
state/api/apiclient.go:131: type AllWatcher struct {
doc comment?

https://codereview.appspot.com/7563046/diff/1/state/api/apiclient.go#newcode137
state/api/apiclient.go:137: return &AllWatcher{
you need just &AllWatcher{client, id} here.

https://codereview.appspot.com/7563046/diff/1/state/api/apiclient.go#newcode143
state/api/apiclient.go:143: func (watcher *AllWatcher) Next()
(*[]params.Delta, error) {
why not return just []params.Delta, error ? nil is a valid slice value,
if the pointer needs to express missing params.

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

https://codereview.appspot.com/7563046/diff/1/state/api/params/params.go#newcode117
state/api/params/params.go:117: func (d *Delta) UnmarshalJSON(data
[]byte) error {
// UnmarshalJSON implements json.Marshaller. ?

https://codereview.appspot.com/7563046/diff/1/state/api/params/params.go#newcode137
state/api/params/params.go:137: return fmt.Errorf("Unexpected operation
%#v", operation)
s/%#v/%q/ ?

https://codereview.appspot.com/7563046/diff/1/state/api/params/params.go#newcode149
state/api/params/params.go:149: return fmt.Errorf("Unexpected entity
name %#v", entityKind)
ditto

https://codereview.appspot.com/7563046/diff/1/state/api/params/params_test.go
File state/api/params/params_test.go (right):

https://codereview.appspot.com/7563046/diff/1/state/api/params/params_test.go#newcode105
state/api/params/params_test.go:105: c.Check(
it'll be nicer to read the code if you assign the json.Unmarshal() to a
var to check (all 3 cases below).

https://codereview.appspot.com/7563046/

« Back to merge proposal