-
Notifications
You must be signed in to change notification settings - Fork 2
merge_join_by #5
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
Conversation
|
I'm on vacation for a couple of weeks and then I can review - sorry! Please bump this PR if I don't get to it :) |
|
Gotcha, enjoy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new merge_join_by combinator for streams, which merges two sorted streams while preserving information about item origins via an EitherOrBoth enum. The implementation includes property-based testing, updates to Rust 2024 edition, dependency updates, and removal of redundant reference operators in test code.
- Adds
merge_join_bystream combinator with comprehensive documentation and examples - Updates to Rust 2024 edition with minimum version bump to 1.85.0
- Cleans up test code by removing unnecessary reference operators with
noop_waker_ref()
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/merge_join_by.rs | New module implementing the MergeJoinBy stream combinator with size hints and property-based tests |
| src/lib.rs | Adds merge_join_by method to StreamTools trait with detailed documentation and examples |
| examples/merge_join_by_trystream.rs | Example demonstrating usage with TryStream and error handling |
| Cargo.toml | Updates edition to 2024, rust-version to 1.85.0, adds either-or-both and quickcheck dependencies |
| CHANGELOG.md | Documents new combinator, rust version bump, and tokio security update |
| README.md | Updates installation instructions to use cargo add command |
| src/sample.rs | Removes unnecessary & operator in Context::from_waker calls |
| src/flatten_switch.rs | Removes unnecessary & operator in Context::from_waker calls |
| src/fast_forward.rs | Removes unnecessary & operator in Context::from_waker calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Looks good to me - just one comment open about a typo. Thanks for the detailed write-up! ✅ |
extremeandy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in "bumb" (bump?) in commit msg but I'm not that fussed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Maybe I should just sacrifice a little time for configuring a spellchecker ^^ |
|
Sorry didn't realise the CI was failing because of that weird feature. Feel free to remove it and then if anything goes awry with the docs I will fix on a separate PR. |
Upstreaming my
merge_join_byimplementation here. This has received some amount of real world testing already.Design choices
EitherOrBoth
We need an
EitherOrBothenum. Instead of writing yet another implementation, I opted to use the one from either_or_both crate. It's fairly new but it has a plethora of useful methods and impls. One usage of this is visible inexamples/merge_join_by_trystream.rs.There might be some breaking changes still coming, but likely these will be limited to the niceties and not the core enum.
Alternatives
itertoolsWe could depend on
itertoolsand use theirEitherOrBothwhich isn't a bad one either. This would be a little unorthodox use ofitertoolsthough.eitherCurrently the well established
eithercrate doesn't have an "OrBoth" version, but there was some talk of adding one in this issue. That would be a great option if it ever comes to fruition.Opaque return type
I've never really understood streams and iterators typically expose the underlying type. Previously I seeked some illumination from
itertools, and it seems like this is mostly due historical reasons.I chose to return an opaque
impl Stream<...>type. It'll be more flexible in the future.Other things done
Rust 2024
I took the opportunity to bump edition to 2024. No changes required, other than a rust minimum version bump.
Some warnings
Some misc warnings here and there. The
CHANNEL_NIGHTLYattribute still causes one.quickcheck
I used to
quickcheckto implement some property testing on the new combinator. Inspired byitertools.Issue refs
fixes: #4