Merge lp://staging/~gary/juju-core/megawatcher_api into lp://staging/~juju/juju-core/trunk

Reviewers: mp+152228_code.launchpad.net,

Message:
Please take a look.

Description:
Exposed as-yet-unimplemented state watcher

Add api server code, api client code, and partly stub tests.

https://code.launchpad.net/~gary/juju-core/megawatcher_api/+merge/152228

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7563046/

Affected files:
   A [revision details]
   M state/api/apiclient.go
   M state/api/params/params.go
   M state/api/params/params_test.go
   M state/apiserver/api_test.go
   M state/apiserver/apiserver.go
   M state/megawatcher.go
   M state/state.go

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/

Please take a look.

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 {
On 2013/03/07 18:05:24, dimitern wrote:
> doc comment?

Done.

https://codereview.appspot.com/7563046/diff/1/state/api/apiclient.go#newcode137
state/api/apiclient.go:137: return &AllWatcher{
On 2013/03/07 18:05:24, dimitern wrote:
> you need just &AllWatcher{client, id} here.

Done.

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

Done.

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 {
On 2013/03/07 18:05:24, dimitern wrote:
> // UnmarshalJSON implements json.Marshaller. ?

Done.

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)
On 2013/03/07 18:05:24, dimitern wrote:
> s/%#v/%q/ ?

Done.

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)
On 2013/03/07 18:05:24, dimitern wrote:
> ditto

Done.

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

Please take a look.

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 {
On 2013/03/07 18:05:24, dimitern wrote:
> doc comment?

Done.

https://codereview.appspot.com/7563046/diff/1/state/api/apiclient.go#newcode137
state/api/apiclient.go:137: return &AllWatcher{
On 2013/03/07 18:05:24, dimitern wrote:
> you need just &AllWatcher{client, id} here.

Done.

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

Done.

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 {
On 2013/03/07 18:05:24, dimitern wrote:
> // UnmarshalJSON implements json.Marshaller. ?

Done.

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)
On 2013/03/07 18:05:24, dimitern wrote:
> s/%#v/%q/ ?

Done.

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)
On 2013/03/07 18:05:24, dimitern wrote:
> ditto

Done.

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

I did this one too. :-)

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(
On 2013/03/07 18:05:24, dimitern wrote:
> it'll be nicer to read the code if you assign the json.Unmarshal() to
a var to
> check (all 3 cases below).

Done.

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

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/

*** Submitted:

Exposed as-yet-unimplemented state watcher

Add api server code, api client code, and partly stub tests. Delta code
had to be moved to the new params location.

R=dimitern, rog
CC=
https://codereview.appspot.com/7563046

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 {
On 2013/03/07 21:52:03, rog wrote:
> 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 :-)

As we discussed, we used "...Results" instead to match the other one.

https://codereview.appspot.com/7563046/diff/6001/state/api/params/params.go#newcode91
state/api/params/params.go:91: type Delta struct {
On 2013/03/07 21:52:03, rog wrote:
> looks like the branch with this code should have been a prereq

As we discussed, this was a code move because of a conflict with some
changes in master.

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
On 2013/03/07 21:52:03, rog wrote:
> 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)

Done.

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