Skip to content

refactor: add doWithResult for easier usage#4136

Merged
chronark merged 4 commits intomainfrom
eng-2012
Oct 27, 2025
Merged

refactor: add doWithResult for easier usage#4136
chronark merged 4 commits intomainfrom
eng-2012

Conversation

@ogzhanolguncu
Copy link
Contributor

What does this PR do?

This PR adds doWithResult to retry pkg for easier usage. Now, we don't have to hoist result var, make closure and deal with shadow err vals. Also fixes and updates retry pkg docs.

Fixes #3829

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Make sure tests are golang tests are passing.

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@linear
Copy link

linear bot commented Oct 22, 2025

@changeset-bot
Copy link

changeset-bot bot commented Oct 22, 2025

⚠️ No Changeset found

Latest commit: fae303c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 22, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
dashboard Ignored Ignored Preview Oct 24, 2025 7:59am
engineering Ignored Ignored Preview Oct 24, 2025 7:59am

@github-actions
Copy link
Contributor

github-actions bot commented Oct 22, 2025

Thank you for following the naming conventions for pull request titles! 🙏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

📝 Walkthrough

Walkthrough

The PR adds a new generic DoWithResult method to the retry package to eliminate the closure variable-capture pattern for result handling. The method is integrated into the db/retry module, and documentation is updated to clarify attempt numbering (1-based) and recommend the New constructor over the deprecated Build alias.

Changes

Cohort / File(s) Change Summary
New retry API method and documentation
go/pkg/retry/retry.go
Introduces generic DoWithResult[T any](r *retry, fn func() (T, error)) (T, error) method for returning typed results from retry operations. Updates backoff documentation to clarify 1-based attempt numbering. Adjusts constructor documentation to prefer New over Build.
Integration with database retry logic
go/pkg/db/retry.go
Replaces manual closure-based result capture pattern with single retry.DoWithResult(retrier, fn) call, preserving retry configuration and error semantics.
Test coverage for DoWithResult
go/pkg/retry/retry_test.go
Adds comprehensive TestDoWithResult function covering success on first attempt, success after multiple retries with accumulated sleep duration tracking, and final failure scenarios with result and error validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "refactor: add doWithResult for easier usage" clearly summarizes the main change in the changeset. It is concise, specific, and directly reflects the primary objective of introducing a new generic DoWithResult method to simplify the retry API usage pattern. The title accurately describes what the developer changed from their perspective without unnecessary verbosity or vague terms.
Linked Issues Check ✅ Passed All primary objectives from linked issue #3829 have been met. The changes add the generic DoWithResult[T any] method to the retry package, update WithRetry to use it internally, maintain backward compatibility by not removing existing APIs, include comprehensive tests for the new functionality via TestDoWithResult, and update documentation by replacing Build references with New and updating backoff documentation. The implementation directly addresses the acceptance criteria specified in the issue.
Out of Scope Changes Check ✅ Passed All changes in the pull request are directly related to the objectives in linked issue #3829. The modifications to go/pkg/db/retry.go demonstrate the new DoWithResult API in use, go/pkg/retry/retry.go introduces the new method and updates documentation as required, and go/pkg/retry/retry_test.go adds comprehensive test coverage. No extraneous changes or modifications unrelated to the stated objectives are present.
Description Check ✅ Passed The pull request description follows the repository template structure and is mostly complete. It includes a clear summary of what the PR does, correctly identifies the type of change as an enhancement, references the linked issue #3829, provides testing instructions, and marks most required checklist items as completed. While some checklist items reference pnpm tools that may not apply to Go-only changes, the essential information needed to understand the PR's purpose and scope is adequately provided.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eng-2012

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c764837 and 772960d.

📒 Files selected for processing (3)
  • go/pkg/db/retry.go (1 hunks)
  • go/pkg/retry/retry.go (4 hunks)
  • go/pkg/retry/retry_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
go/pkg/db/retry.go (1)
go/pkg/retry/retry.go (1)
  • DoWithResult (170-178)
go/pkg/retry/retry_test.go (1)
go/pkg/retry/retry.go (2)
  • New (63-74)
  • DoWithResult (170-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build / Build
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
go/pkg/db/retry.go (1)

59-59: LGTM! Clean refactoring using the new DoWithResult helper.

This change successfully eliminates the manual closure pattern while preserving the exact same retry semantics. The generic type T is correctly inferred from the WithRetry function signature.

go/pkg/retry/retry_test.go (1)

225-294: LGTM! Comprehensive test coverage for DoWithResult.

The test thoroughly validates the new generic helper across three critical scenarios:

  • Immediate success without retries
  • Success after multiple retry attempts
  • Complete failure returning the last attempt's result

The test correctly validates call counts, accumulated sleep duration, result values, and error propagation.

go/pkg/retry/retry.go (3)

15-15: LGTM! Helpful clarification on attempt numbering.

The documentation update correctly clarifies that the backoff function receives attempt numbers starting at 1, which aligns with the implementation and helps users write correct backoff functions.


27-62: LGTM! Documentation modernization.

The updated examples correctly use New() constructor instead of the deprecated Build() alias, providing users with current best practices.


157-178: LGTM! DoWithResult implementation is correct and well-documented.

The implementation properly:

  • Uses a closure to capture the result variable, eliminating the awkward pattern users had to write themselves
  • Avoids variable shadowing by using retryErr inside the closure
  • Returns the result from the last attempt on failure, as documented
  • Delegates to Do() for consistent retry semantics

The generic constraint [T any] is appropriate and the function signature matches the PR objectives.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@vercel vercel bot temporarily deployed to Preview – engineering October 23, 2025 12:38 Inactive
@chronark chronark added this pull request to the merge queue Oct 27, 2025
@graphite-app
Copy link

graphite-app bot commented Oct 27, 2025

Illustrated gif. A hand appears and holds up an oversized thumb, giving us a thumbs up. (Added via Giphy)

@graphite-app
Copy link

graphite-app bot commented Oct 27, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (10/27/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Merged via the queue into main with commit beb15c2 Oct 27, 2025
18 checks passed
@chronark chronark deleted the eng-2012 branch October 27, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve retry API: Add generic DoWithResult method to avoid closure pattern

3 participants