Code review comment for lp:~themue/juju-core/024-cleanup-watcher

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

LGTM with a few comments changed.

https://codereview.appspot.com/10078043/diff/14002/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcode1363
state/state_test.go:1363: c.Fatalf("unexpected change: %v", ok)
please change this to say "unexpected change; ok: %v"

https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcode1383
state/state_test.go:1383: // Add relations doesn't emit events.
// Adding relations doesn't emit events.

https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcode1415
state/state_test.go:1415: // A cleanup without need doesn't emit events.
// Verify that Cleanup() doesn't emit unnecessary events.

https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcode1420
state/state_test.go:1420: // Don't handle each event keeps them in the
queue. Cleanups
// Not calling Cleanup() queues up the changes, so that
// multiple changes will be emitted the next time Cleanup()
// is called. This behavior will change in a follow-up.

https://codereview.appspot.com/10078043/diff/14002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1251
state/watcher.go:1251: // CleanupWatcher notifies changes of the cleanup
documents
// CleanupWatcher notifies of changes in the cleanups collection.
?

https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1258
state/watcher.go:1258: // WatchCleanups returns a CleanupWatcher that
notifies when documents
// WatchCleanups starts and returns a CleanupWatcher.

https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1289
state/watcher.go:1289: w.out <- struct{}{}
nice!

https://codereview.appspot.com/10078043/

« Back to merge proposal