7966: impl Stream for std::collections::*#77
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 significantly enhances the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request adds support for collecting streams into additional standard library container types. Implementations of ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![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 adds FromStream implementations for several standard library collections, allowing them to be used with StreamExt::collect. The new collections supported are VecDeque, LinkedList, BTreeMap, HashSet, HashMap, and BinaryHeap. The implementations correctly use size hints for allocation where possible and are accompanied by a comprehensive set of tests. My feedback focuses on improving the maintainability of the newly added test code by reducing duplication.
| async fn collect_vecdeque_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<VecDeque<i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(1).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(2).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(VecDeque::from([1, 2]), coll); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn collect_linkedlist_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<LinkedList<i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(1).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(2).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(LinkedList::from([1, 2]), coll); | ||
| } |
There was a problem hiding this comment.
These new tests for VecDeque and LinkedList are very similar and contain a lot of boilerplate, a pattern that is repeated for the other collections added in this PR. To improve maintainability and reduce this duplication, consider using a macro to generate the test cases.
This would make the test suite more concise and easier to extend. I've added a similar comment on the other block of new tests with a concrete example.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! There is a lot of duplicated logic that could be extracted into a macro and reused for all collection types. This would prevent fixing/improving the same thing multiple times.
| async fn collect_btreemap_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<BTreeMap<i32, i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send((3, 4)).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send((1, 2)).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(BTreeMap::from([(1, 2), (3, 4)]), coll); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn collect_hashset_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<HashSet<i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(1).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(2).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(HashSet::from([1, 2]), coll); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn collect_hashmap_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<HashMap<i32, i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send((1, 2)).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send((3, 4)).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(HashMap::from([(1, 2), (3, 4)]), coll); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn collect_binaryheap_items() { | ||
| let (tx, rx) = mpsc::unbounded_channel_stream(); | ||
| let mut fut = task::spawn(rx.collect::<BinaryHeap<i32>>()); | ||
|
|
||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(2).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| tx.send(1).unwrap(); | ||
| assert!(fut.is_woken()); | ||
| assert_pending!(fut.poll()); | ||
|
|
||
| drop(tx); | ||
| assert!(fut.is_woken()); | ||
| let coll = assert_ready!(fut.poll()); | ||
| assert_eq!(vec![1, 2], coll.into_sorted_vec()); | ||
| } |
There was a problem hiding this comment.
As mentioned for the other new tests, there is significant code duplication here. Using a macro would greatly improve the maintainability of these tests.
Here is an example of how such a macro could look and be used:
macro_rules! test_collect {
($name:ident, $ty:ty, $items:expr, $expected:expr) => {
#[tokio::test]
async fn $name() {
let (tx, rx) = mpsc::unbounded_channel_stream();
let mut fut = task::spawn(rx.collect::<$ty>());
assert_pending!(fut.poll());
for item in $items {
tx.send(item).unwrap();
assert!(fut.is_woken());
assert_pending!(fut.poll());
}
drop(tx);
assert!(fut.is_woken());
let coll = assert_ready!(fut.poll());
assert_eq!($expected, coll);
}
};
}
// Usage:
test_collect!(
collect_btreemap_items,
BTreeMap<i32, i32>,
vec![(3, 4), (1, 2)],
BTreeMap::from([(1, 2), (3, 4)])
);Tests with special assertion logic, like for BinaryHeap, might need to remain as separate functions.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! There is a lot of duplicated logic that could be extracted into a macro and reused for all collection types. This would prevent fixing/improving the same thing multiple times.
🤖 Augment PR SummarySummary: This PR expands Changes:
Technical Notes: Map/set implementations use 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Code ReviewOverall this is a clean, well-structured PR that extends FromStream to the remaining standard library collection types. The implementations are correct, consistent with existing patterns, and well-tested. PR Title Misnomer: The title says impl Stream for std::collections but the trait being implemented is FromStream, not Stream. Stream is a producer, FromStream is a consumer/collector. Consider updating the PR title accordingly. Missing CHANGELOG Entry: tokio-stream/Cargo.toml explicitly notes Update CHANGELOG.md as a pre-release step. This PR adds six new FromStream impls and should include a CHANGELOG entry. Undocumented Duplicate Key Behavior: For HashMap and BTreeMap, inserting a duplicate key silently overwrites the previous value (last write wins). A test covering the duplicate-key case would make the behavior clear to future readers. Missing Empty-Stream Tests (non-blocking): The existing suite has empty_vec, empty_box_slice, empty_string, and empty_result. A few empty-stream tests for the new types would align with the existing test style. What Looks Good: All six implementations correctly follow the sealed FromStreamPriv pattern. with_capacity(lower) is used where available and omitted where not. The BinaryHeap test uses into_sorted_vec() for deterministic assertion. The BTreeMap test sends keys out-of-order and asserts the sorted result. std::hash::Hash is correctly added for HashSet and HashMap impls. Tests for all six new types follow the exact polling/wakeup pattern used by existing tests. Summary: The implementation is correct and well-structured. The main items to address are the PR title and the missing CHANGELOG entry. Documenting or testing the duplicate-key behavior for map types would also be a worthwhile improvement. |
2 participants
7966: To review by AI