Skip to content

Added RetryWithCallback allowing cb to terminate early with message#360

Closed
morganhein wants to merge 3 commits intogo-kit:masterfrom
morganhein:retry_with_cb
Closed

Added RetryWithCallback allowing cb to terminate early with message#360
morganhein wants to merge 3 commits intogo-kit:masterfrom
morganhein:retry_with_cb

Conversation

@morganhein
Copy link
Copy Markdown

#341

Love them callbacks.

@peterbourgon
Copy link
Copy Markdown
Member

Your gofmt seems to be broken; it's inserting spaces instead of tabs?

Comment thread sd/lb/retry.go
// error message if desired. The error message may be nil, but a true/false
// is always expected. In all cases if the error message is supplied, the
// current error will be replaced.
type Callback func(int, string) (bool, *string)
Copy link
Copy Markdown
Member

@peterbourgon peterbourgon Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, a pointer to a string is not a great way to encode maybe-error. It would make a lot more sense if Callback had the signature

type Callback func(n int, err error) (continue bool, err error)

Is there a reason that's not possible?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I should've done that in the first place. Still learning obv. I'll fix spaces/tabs and the signature.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just gave this signature a try. I think the named return params help legibility, but continue is a keyword, so you'll probably have to go with cont.

@peterbourgon
Copy link
Copy Markdown
Member

Ping? :)

@rossmcf
Copy link
Copy Markdown
Contributor

rossmcf commented Oct 2, 2016

I agree with @peterbourgon. Allowing the callback to receive an error would make it easier to handle the case described in #341, where you want to check for a particular error type.

@peterbourgon
Copy link
Copy Markdown
Member

Looks like this has been abandoned. I'm closing in favor of #370, but please correct me if I've got it wrong :) Thanks for the initiative! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants