-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DoWithData function #91
Conversation
Hi @craigpastro This looks good to me. I was thinking about generic and this library, and I have not seen any reason why, but this is good reason. I've just looked at the golang support https://endoflife.date/go and I suppose it's ok to only support Go 1.18+. Please keep both examples in the documentation. |
Hi @JaSei 👋 That's great. It will be nice to get this functionality here. I think you are right about supporting only 1.18+. I am thinking about what to do with the tests. I could create two of each for |
Hi again @JaSei. I updated the CI to only run with Go versions 1.18 to 1.20 and also to run on pull request. Please let me know if that is okay. Thanks! Also, would you like to update the examples in |
I'm also interested in this. Any chance to get it merged? |
I apologize for the long silence. I'm back now. It looks good to me. |
I'm just afraid of performance, because generics affects performance. That will be awesome to do benchmark before and with this change 🤔 |
I've just merged #89 where some benchmark are available |
👋 @JaSei. Welcome back. I merged main into my branch so we can take a look at the benchmarks 👍 |
I added similar
So the generic version is actually faster on my computer 🤷 |
|
current
generic
benchstat
|
I'm thinking about version. |
Oh. haha. How embarrassing. I obviously wasn't thinking.
That is a good question and those are valid points. In my opinion, with the Go policy of just maintaining the last two releases, bumping the minor version is probably fine. |
Thanks a lot for the PR @craigpastro. Good job |
Very nice improvement! Thanks! |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/avast/retry-go/v4](https://github.com/avast/retry-go) | require | minor | `v4.3.4` -> `v4.5.0` | --- ### Release Notes <details> <summary>avast/retry-go (github.com/avast/retry-go/v4)</summary> ### [`v4.5.0`](https://github.com/avast/retry-go/releases/tag/v4.5.0) [Compare Source](https://github.com/avast/retry-go/compare/4.4.0...4.5.0) #### What's Changed - Allow last error to be returned with context error by [@​willdot](https://github.com/willdot) in [avast/retry-go#96 #### New Contributors - [@​willdot](https://github.com/willdot) made their first contribution in [avast/retry-go#96 **Full Changelog**: avast/retry-go@4.4.0...v4.5.0 ### [`v4.4.0`](https://github.com/avast/retry-go/releases/tag/v4.4.0): (generic support) [Compare Source](https://github.com/avast/retry-go/compare/4.3.4...4.4.0) #### What's Changed - Go versions by [@​JaSei](https://github.com/JaSei) in [avast/retry-go#97 - fix: markdown code block format by [@​mrtc0](https://github.com/mrtc0) in [avast/retry-go#93 - remove error log pre-allocation and add benchmark by [@​dillonstreator](https://github.com/dillonstreator) in [avast/retry-go#89 - Add DoWithData function by [@​craigpastro](https://github.com/craigpastro) in [avast/retry-go#91 #### New Contributors - [@​mrtc0](https://github.com/mrtc0) made their first contribution in [avast/retry-go#93 - [@​dillonstreator](https://github.com/dillonstreator) made their first contribution in [avast/retry-go#89 - [@​craigpastro](https://github.com/craigpastro) made their first contribution in [avast/retry-go#91 **Full Changelog**: avast/retry-go@4.3.4...v4.4.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43OC4zIiwidXBkYXRlZEluVmVyIjoiMzYuNzguMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
👋 I am not sure that you will consider this PR, but it is to add a
DoWithData
generic function that can also return data from a retry, not just an error. That is, it resolves #58. Of course, with this change I had to bump to Go 1.18.If you think this is a useful feature, I could add some more tests and documentation.
I wasn't sure what to name this function so I borrowed the idea from https://pkg.go.dev/github.com/cenkalti/backoff/v4, which has a
RetryWithData
function which does a similar thing as this.Thanks for the very useful library ❤️