Simpleworker implemention

Simpleworker implemention https://code.launchpad.net/~natefinch/juju-core/036-simpleworker/+merge/207725 (do not edit description out of merge proposal)

[natefinch, 2014-02-21 19:11:37]

Please take a look.

[natefinch, 2014-02-21 19:12:43]

On 2014/02/21 19:11:37, nate.finch wrote: > Please take a look. Forgot to mention in the message... this is a simplified worker that makes it easy to turn a small amount of code into a worker without having to create a lot of boilerplate. It'll be used in the mongo HA stuff, but committing early so ian can do it.

[natefinch, 2014-02-21 19:15:44]

On 2014/02/21 19:12:43, nate.finch wrote: > On 2014/02/21 19:11:37, nate.finch wrote: > > Please take a look. > > Forgot to mention in the message... this is a simplified worker that makes it > easy to turn a small amount of code into a worker without having to create a lot > of boilerplate. It'll be used in the mongo HA stuff, but committing early so > ian can do it. s/do it/use it/

[rog, 2014-02-24 17:57:35]

LGTM with a couple of suggestions below, thanks. https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go File worker/simpleworker.go (right): https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go#newcode7 worker/simpleworker.go:7: // mechanisms through the use of a couple channels. The channels are only used A slightly different suggestion: // simpleWorker implements the worker returned by NewSimpleWorker. // The stopc and done channels are used for closing notifications // only. No values are sent over them. The err value is set once only, // just before the done channel is closed. ? https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go File worker/simpleworker_test.go (right): https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go#ne... worker/simpleworker_test.go:39: c.Assert(w.Wait(), gc.Equals, nil) This isn't quite testing that we're waiting until doWork returns (perhaps we're not actually seeing the value returned by doWork at all). If we returned testError from the function, the test would be more convincing. Then perhaps one more test to check that the happy path works.

[natefinch, 2014-02-24 19:26:43]

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go File worker/simpleworker.go (right): https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go#newcode7 worker/simpleworker.go:7: // mechanisms through the use of a couple channels. The channels are only used On 2014/02/24 17:57:36, rog wrote: > A slightly different suggestion: > > // simpleWorker implements the worker returned by NewSimpleWorker. > // The stopc and done channels are used for closing notifications > // only. No values are sent over them. The err value is set once only, > // just before the done channel is closed. > > ? Sounds good. Done. https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go File worker/simpleworker_test.go (right): https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go#ne... worker/simpleworker_test.go:39: c.Assert(w.Wait(), gc.Equals, nil) On 2014/02/24 17:57:36, rog wrote: > This isn't quite testing that we're waiting until doWork > returns (perhaps we're not actually seeing the value > returned by doWork at all). If we returned testError from the function, > the test would be more convincing. Good point. Done. > Then perhaps one more test to check that the > happy path works. Added a test that nil gets passed through Wait().