refactor: update ClientBuilder to return a JoinHandle#21
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ClientBuilder to return an additional JoinHandle, allowing the EventStream to be used with more flexibility while giving the user control over the background task managing the WebSocket connection.
- Updated build_client API in tests and common modules to return a JoinHandle.
- Refactored EventStream in src/lib.rs: removed the automatic task abort on drop and replaced it with a Stream trait implementation.
- Updated examples and Cargo.toml to accommodate the new dependency and API changes.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration.rs | Updated the usage of build_client to destructure the extra JoinHandle and explicitly abort the task. |
| tests/common/mod.rs | Updated build_client signature to return a JoinHandle. |
| src/lib.rs | Modified ClientBuilder::build, updated EventStream and removed the Drop impl. |
| examples/text_to_image.rs | Adapted ClientBuilder usage to capture and abort the JoinHandle. |
| Cargo.toml | Added pin-project-lite dependency for the new EventStream implementation. |
Comments suppressed due to low confidence (2)
src/lib.rs:238
- [nitpick] Consider renaming 'stream_handle' to a more consistent name like 'handle' or 'join_handle' to match its usage in other parts of the code, such as in the tests.
Ok((client, stream, stream_handle))
src/lib.rs:514
- The removal of the Drop implementation for EventStream means that the background task is no longer automatically aborted when the stream is dropped. Ensure that this design change is clearly documented so that users understand they must explicitly call abort on the returned JoinHandle to prevent lingering tasks.
impl Drop for EventStream {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I refactored it this way to allow
EventStreamto useSteamExt's cool methods, such as map, filter, etc., while allowing users to manipulate Handles to increase flexibility