ChildrenWatcher now treats missing nodes as having no children

ChildrenWatcher now treats missing nodes as having no children

This is consistent with ContentWatcher (which returns {false, ""} content
changes on missing nodes) and enables simpler code when adding relations (by
deferring role node creation until unit add time in all cases, rather than
just for container-scoped relations).

https://code.launchpad.net/~fwereade/juju-core/tweak-children-watcher/+merge/109630

(do not edit description out of merge proposal)

[fwereade, 2012-06-11 13:15:18]

Please take a look.

[niemeyer, 2012-06-13 11:22:54]

LGTM, with a couple of suggestions:

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go#newcode24
state/watcher/watcher.go:24: started    bool
Are you just looking to invert the boolean meaning here? If so, can we preserve
the context of the name with something like "emittedValue"?  "started" is a
weaker term, and potentially confusing given that we have a Stop method, that is
not entirely related to the relevancy of this field.

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go#newcod...
state/watcher/watcher.go:241: return
Would you mind to invert the logic so that the reason why we're returning here
is more obvious?

if err != nil {
        return
}
break

Then drop the if err == nil at the top of the loop.

[fwereade, 2012-06-14 14:26:04]

*** Submitted:

ChildrenWatcher now treats missing nodes as having no children

This is consistent with ContentWatcher (which returns {false, ""} content
changes on missing nodes) and enables simpler code when adding relations (by
deferring role node creation until unit add time in all cases, rather than
just for container-scoped relations).

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

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go#newcode24
state/watcher/watcher.go:24: started    bool
On 2012/06/13 11:22:54, niemeyer wrote:
> Are you just looking to invert the boolean meaning here? If so, can we
preserve
> the context of the name with something like "emittedValue"?  "started" is a
> weaker term, and potentially confusing given that we have a Stop method, that
is
> not entirely related to the relevancy of this field.

SGTM, thanks.

https://codereview.appspot.com/6301066/diff/1/state/watcher/watcher.go#newcod...
state/watcher/watcher.go:241: return
On 2012/06/13 11:22:54, niemeyer wrote:
> Would you mind to invert the logic so that the reason why we're returning here
> is more obvious?
> 
> if err != nil {
>         return
> }
> break
> 
> Then drop the if err == nil at the top of the loop.

Done.