environs: make AttemptStrategy public

environs: make AttemptStrategy public

I would like to reuse the AttemptStrategy code in the PA to 
implement a retry loop at the top level. After a short 
discussion on IRC, no good places for this code were found, 
but environs.AttemptStrategy appeared to be the least worse 
option until a better option arrives.

https://code.launchpad.net/~dave-cheney/juju-core/go-juju-short-attempt/+merge/109744

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-06-11 22:41:09]

Please take a look.

[rog, 2012-06-12 08:12:30]

LGTM modulo the below. i'm not that happy about cluttering up environs, but it
really doesn't justify a new package (though "attempt.Strategy" reads well).

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go
File environs/attempt.go (right):

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode14
environs/attempt.go:14: type attempt struct {
this should be exported. it's not nice returning types that can't be declared.

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode19
environs/attempt.go:19: func (a AttemptStrategy) Start() *attempt {
// Start begins a new sequence of attempts with
// the given strategy.
?

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode25
environs/attempt.go:25: func (a *attempt) Next() bool {
// Next waits until it is time to perform
// the next attempt and returns whether
// there are any more attempts left.
?

[dave_cheney.net, 2012-06-12 08:20:34]

Agreed the placement in the environs package is less than optimal. In
discussions on IRC Gustavo and I couldn't think of a better place, and a utils
package was the beginning of the end. Given that, this was the least worst
option. 

On 12/06/2012, at 18:12, rogpeppe@gmail.com wrote:

> LGTM modulo the below. i'm not that happy about cluttering up environs,
> but it really doesn't justify a new package (though "attempt.Strategy"
> reads well).
> 
> 
> https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go
> File environs/attempt.go (right):
> 
>
https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode14
> environs/attempt.go:14: type attempt struct {
> this should be exported. it's not nice returning types that can't be
> declared.
> 
>
https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode19
> environs/attempt.go:19: func (a AttemptStrategy) Start() *attempt {
> // Start begins a new sequence of attempts with
> // the given strategy.
> ?
> 
>
https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode25
> environs/attempt.go:25: func (a *attempt) Next() bool {
> // Next waits until it is time to perform
> // the next attempt and returns whether
> // there are any more attempts left.
> ?
> 
> https://codereview.appspot.com/6294065/

[dave_cheney.net, 2012-06-12 22:53:38]

Please take a look.

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go
File environs/attempt.go (right):

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode14
environs/attempt.go:14: type attempt struct {
On 2012/06/12 08:12:30, rog wrote:
> this should be exported. it's not nice returning types that can't be declared.

I feel no shame, what is your objection ?

If this type was exported, AttemptStrategy should no remain an embedded struct.

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode25
environs/attempt.go:25: func (a *attempt) Next() bool {
On 2012/06/12 08:12:30, rog wrote:
> // Next waits until it is time to perform
> // the next attempt and returns whether
> // there are any more attempts left.
> ?

Done.

[niemeyer, 2012-06-13 01:43:16]

LGTM, with trivials:

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go
File environs/attempt.go (right):

https://codereview.appspot.com/6294065/diff/2001/environs/attempt.go#newcode14
environs/attempt.go:14: type attempt struct {
On 2012/06/12 22:53:38, dfc wrote:
> On 2012/06/12 08:12:30, rog wrote:
> > this should be exported. it's not nice returning types that can't be
declared.
> 
> I feel no shame, what is your objection ?
> 
> If this type was exported, AttemptStrategy should no remain an embedded
struct.

I agree with both of you. It should be exported, and AttemptStrategy should be a
"strategy" field instead. There's no reason for it to be public, or for its
methods to be available on Attempt (attempt.Start()?).

https://codereview.appspot.com/6294065/diff/7001/environs/attempt.go
File environs/attempt.go (right):

https://codereview.appspot.com/6294065/diff/7001/environs/attempt.go#newcode27
environs/attempt.go:27: // false if not attempts remain.
// false if it's time to stop trying.

That seems more clear and fixes the "not attempts" typo.

https://codereview.appspot.com/6294065/diff/7001/environs/internal_test.go
File environs/internal_test.go (right):

https://codereview.appspot.com/6294065/diff/7001/environs/internal_test.go#ne...
environs/internal_test.go:1: package environs
package environs_test

and the wording below should be changed. There's no reason for this to be an
internal suite anymore since we're dealing with public types.

https://codereview.appspot.com/6294065/diff/7001/environs/internal_test.go#ne...
environs/internal_test.go:10: var _ = Suite(internalSuite{})
AttemptSuite or similar

[dave_cheney.net, 2012-06-13 01:44:10]

Thanks, I am having a bad day for typos, it is very cold in this room.

[dave_cheney.net, 2012-06-13 02:44:59]

*** Submitted:

environs: make AttemptStrategy public

I would like to reuse the AttemptStrategy code in the PA to 
implement a retry loop at the top level. After a short 
discussion on IRC, no good places for this code were found, 
but environs.AttemptStrategy appeared to be the least worse 
option until a better option arrives.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6294065

https://codereview.appspot.com/6294065/diff/7001/environs/attempt.go
File environs/attempt.go (right):

https://codereview.appspot.com/6294065/diff/7001/environs/attempt.go#newcode27
environs/attempt.go:27: // false if not attempts remain.
On 2012/06/13 01:43:16, niemeyer wrote:
> // false if it's time to stop trying.
> 
> That seems more clear and fixes the "not attempts" typo.

Done.

https://codereview.appspot.com/6294065/diff/7001/environs/internal_test.go
File environs/internal_test.go (right):

https://codereview.appspot.com/6294065/diff/7001/environs/internal_test.go#ne...
environs/internal_test.go:1: package environs
On 2012/06/13 01:43:16, niemeyer wrote:
> package environs_test
> 
> and the wording below should be changed. There's no reason for this to be an
> internal suite anymore since we're dealing with public types.

Done.