Code review comment for lp:~thumper/juju-core/refactor-uniter-tests

Revision history for this message
Tim Penhey (thumper) wrote :

Please take a look.

https://codereview.appspot.com/41210043/diff/1/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/context.go#newcode191
worker/uniter/context.go:191: writeChanges := err == nil
On 2013/12/13 00:23:11, wallyworld wrote:
> Why not just make writeChanges a method param?
> And set it accordingly in the caller if the err the caller has is nil?

Because doing it this way makes the caller much cleaner, and we need the
error anyway as we return that if it is not-nil.

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter.go#newcode35
worker/uniter/uniter.go:35: type UniterExecutionObserver interface {
On 2013/12/13 00:23:11, wallyworld wrote:
> Doc string please

Done.

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (left):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter_test.go#oldcode192
worker/uniter/uniter_test.go:192: func (ctx *context) matchLogHooks(c
*gc.C) (match bool, overshoot bool) {
On 2013/12/13 00:23:11, wallyworld wrote:
> Is this still the best method name?

Haha, ah, no. I'll change to something more meaningful.

https://codereview.appspot.com/41210043/

« Back to merge proposal