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

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with one or two trivials below. thanks!

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

https://codereview.appspot.com/7563046/diff/6001/state/api/params/params.go#newcode68
state/api/params/params.go:68: type AllWatcherNext struct {
i've been trying to use the convention that params have no suffix, but
results have a Result suffix.

so i'd name this AllWatcherNextResult.
long but consistent :-)

https://codereview.appspot.com/7563046/diff/6001/state/api/params/params.go#newcode91
state/api/params/params.go:91: type Delta struct {
looks like the branch with this code should have been a prereq

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

https://codereview.appspot.com/7563046/diff/6001/state/api/params/params_test.go#newcode10
state/api/params/params_test.go:10: // TestPackage integrates the tests
into gotest.
same here.

https://codereview.appspot.com/7563046/diff/6001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7563046/diff/6001/state/apiserver/api_test.go#newcode1000
state/apiserver/api_test.go:1000: stopped := false
rather than having the bool, we could probably do:
defer func)() {
    err := watcher.Stop()
    c.Check(err, IsNil)
}()

and lose the Stop at the end of the function (and its comment)

https://codereview.appspot.com/7563046/diff/6001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7563046/diff/6001/state/megawatcher.go#newcode25
state/megawatcher.go:25: var StubNextDelta = []params.Delta{
s/Stub/stub/
(or do you want it accessible from tests outside state?)

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

« Back to merge proposal