Enhanced ContentWatcher to watch also creation and deletion.

Enhanced ContentWatcher to watch also creation and deletion.

Instead of a simple content string now a struct with an additional
bool field for existance is emitted.

https://code.launchpad.net/~themue/juju/go-state-enhanced-content-watcher/+merge/102365

(do not edit description out of merge proposal)

[TheMue, 2012-04-17 17:59:38]

Please take a look.

[TheMue, 2012-04-17 18:45:42]

Please take a look.

[niemeyer, 2012-04-18 19:03:24]

Code looks great. LGTM assuming that the following simplifications are done.

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher.go#new...
state/watcher/watcher.go:120: if exists {
Isn't this variable entirely equivalent to (stat != nil) here? If so, let's
please simplify further.

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.go
File state/watcher/watcher_test.go (right):

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.g...
state/watcher/watcher_test.go:77: // delete and create again.
This can be expressed in a more compact and readable form as something similar
to the following (untested).

var expectedChanges = []*watcher.ContentChange{
    {true, "init"},
    {true, "foo"},
    {false, ""},
    {true, "done"},
}
for _, want := range expectedChanges {
        select {
        case got, ok := <-w.Changes():
            c.Assert(ok, Equals, true)
            c.Assert(got, Equals, want)
        case <-time.After(200 * time.Millisecond):
            c.Fatalf("didn't get change: %#v", want)
        }
}
select {
case got, ok := <-w.Changes():
        c.Fatalf("got unexpected change: %#v", got)
case <-time.After(200 * time.Millisecond):
}

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.g...
state/watcher/watcher_test.go:138: change, ok, timeout := receiveChange(watcher)
Please refactor as per suggestion above.

[TheMue, 2012-04-19 10:53:31]

Please take a look.

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher.go#new...
state/watcher/watcher.go:120: if exists {
On 2012/04/18 19:03:25, niemeyer wrote:
> Isn't this variable entirely equivalent to (stat != nil) here? If so, let's
> please simplify further.

Done.

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.go
File state/watcher/watcher_test.go (right):

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.g...
state/watcher/watcher_test.go:77: // delete and create again.
On 2012/04/18 19:03:25, niemeyer wrote:
> This can be expressed in a more compact and readable form as something similar
> to the following (untested).
> 
> var expectedChanges = []*watcher.ContentChange{
>     {true, "init"},
>     {true, "foo"},
>     {false, ""},
>     {true, "done"},
> }
> for _, want := range expectedChanges {
>         select {
>         case got, ok := <-w.Changes():
>             c.Assert(ok, Equals, true)
>             c.Assert(got, Equals, want)
>         case <-time.After(200 * time.Millisecond):
>             c.Fatalf("didn't get change: %#v", want)
>         }
> }
> select {
> case got, ok := <-w.Changes():
>         c.Fatalf("got unexpected change: %#v", got)
> case <-time.After(200 * time.Millisecond):
> }

Done.

https://codereview.appspot.com/6059044/diff/2001/state/watcher/watcher_test.g...
state/watcher/watcher_test.go:138: change, ok, timeout := receiveChange(watcher)
On 2012/04/18 19:03:25, niemeyer wrote:
> Please refactor as per suggestion above.

Done.

[niemeyer, 2012-04-19 10:56:13]

LGTM, thank you!

[TheMue, 2012-04-19 11:10:49]

*** Submitted:

Enhanced ContentWatcher to watch also creation and deletion.

Instead of a simple content string now a struct with an additional
bool field for existance is emitted.

R=niemeyer
CC=
https://codereview.appspot.com/6059044