fn: move Future/Promise from actor package#10727
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the codebase architecture by relocating generic asynchronous primitives from the actor-specific package to a more appropriate, shared utility package. This change promotes better code reuse and separation of concerns by ensuring that these primitives are not coupled to actor-specific functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🟡 PR Severity: MEDIUM
🟡 Medium (8 files)
🟢 Low (1 file)
AnalysisThis PR modifies To override, add a |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the Future and Promise types from the actor package to the fn package to serve as general-purpose async primitives, updating all internal references and module dependencies. Review feedback recommends renaming interface parameters to avoid shadowing the fn package name and reclassifying the change as a breaking change in the release notes for better visibility.
| // returned. If the passed context is cancelled while waiting for the | ||
| // original future to complete, the new future will complete with the | ||
| // context's error. | ||
| ThenApply(ctx context.Context, fn func(T) T) Future[T] |
There was a problem hiding this comment.
The parameter name fn shadows the package name fn. This can lead to confusion and compilation errors if the package needs to be referenced within an implementation of this interface. It is better to use a more descriptive name like fApply, which is already used in the implementation.
| ThenApply(ctx context.Context, fn func(T) T) Future[T] | |
| ThenApply(ctx context.Context, fApply func(T) T) Future[T] |
| // future is ready. If the passed context is cancelled before the future | ||
| // completes, the callback function will be invoked with the context's | ||
| // error. | ||
| OnComplete(ctx context.Context, fn func(Result[T])) |
There was a problem hiding this comment.
The parameter name fn shadows the package name fn. Using a name like cFunc (as seen in the implementation) would avoid this shadowing and improve clarity.
| OnComplete(ctx context.Context, fn func(Result[T])) | |
| OnComplete(ctx context.Context, cFunc func(Result[T])) |
| * [`Future` and `Promise` types have been moved](https://github.com/lightningnetwork/lnd/pull/10727) | ||
| from the `actor` package into the `fn` package, where they better belong as | ||
| general-purpose async primitives. |
There was a problem hiding this comment.
Moving the Future and Promise types from the actor package to the fn package is a breaking change for any external consumers of these packages. This entry should be moved from the 'Code Health' section to the 'Breaking Changes' section (starting around line 195) to ensure it is properly noticed by developers.
Abdulkbk
force-pushed
the
move-to-fn
branch
2 times, most recently
from
643f23e to
bbc0f12
Compare
|
@Abdulkbk, remember to re-request review from reviewers when ready |
Moves the Future[T] and Promise[T] interfaces along with their concrete implementations and NewPromise[T] constructor from the actor package into fn, where they belong as general-purpose async computation primitives.
Internal callers are updated to use fn.NewPromise directly.
Temporary replace directive to build against the local fn/v2 until the Future/Promise changes are published as a new release.
3 participants
Closes #10724.
Change Description
Future[T],Promise[T], andNewPromise[T]currently live in theactorpackage but do not depend on actor-specific functionality.This PR moves them to the
fnpackage where they belong as general-purpose async primitives.Testing
📝 Please see our Contribution Guidelines for further guidance.