Open VorpalBlade opened 5 days ago
Similarly to what we did with WithPosition
, I would suggest (Comm, T)
over Comm<T>
.
We have PutBack
that can be used instead of Peekable
to avoid "expect"s.
Then it makes me think of MergeJoinBy
.
Have you tried to use .merge_join_by(Ord::cmp)
? Maybe it would be too convoluted to use?
Similarly to what we did with
WithPosition
, I would suggest(Comm, T)
overComm<T>
.
Is this to reduce code duplication due to monomorphization? That is the only reason to prefer one over the other that I can think of.
We have
PutBack
that can be used instead ofPeekable
to avoid "expect"s.
Oh cool, we live and learn.
Then it makes me think of
MergeJoinBy
. Have you tried to use.merge_join_by(Ord::ord)
? Maybe it would be too convoluted to use?
It was too convoluted for me to find that. I kept looking for names like "diff", "common", "comm", etc. But looking at the example for merge_join_by it seems to fit. That Both returns, well, both values instead of de-duplicating is not something I need, but it should be fine (I'll just throw one away)
To get the T
value out of Comm<T>
, we need a match
so an additional method, vs .1
. Maybe there was other arguments. Don't remember right now.
If it fits but is a bit convulated, then maybe we can have create a method wrapping this .merge_join_by(Ord::cmp)
.
To get the
T
value out ofComm<T>
, we need amatch
so an additional method, vs.1
. Maybe there was other arguments. Don't remember right now.
Thank you, it is always good to learn about idiomatic API design.
If it fits but is a bit convulated, then maybe we can have create a method wrapping this
.merge_join_by(Ord::cmp)
.
Using it is not convoluted, it works very well. What was convoluted was finding it. Perhaps adding some rustdoc aliases might help people locate it. I would suggest comm
and diff
:
comm
command (three streams of elements).I did not encounter any real use of doc aliases before, that's interesting.
I see the interest of having #[doc(alias = "comm", alias = "diff")] fn merge_join_by...
.
But I'm not sure it's enough, we could have an additional example in the doc of merge_join_by
, what do you think? Do you want to make one?
@jswrenn @phimuemue Any objection to use doc aliases?
PS: I note that doc aliases requires Rust 1.48 and we just increased the MSRV from 1.43.1 to 1.63 so it can be used!
But I'm not sure it's enough, we could have an additional example in the doc of merge_join_by, what do you think? Do you want to make one?
It seems the current first example matches my situation (though it is using an explicit closure rather than just using Ord::cmp
directly (which also works), and since I have clippy::redundant_closure_for_method_calls
enabled it would suggest I switch to that instead).
Maybe it could be made more obvious with a comment (since it uses a range for one of the inputs, so it is not as obvious what it is doing). Something like:
itertools::assert_equal(
// This performs a diff in the style of the Unix command comm(1), generalised
// to arbitrary types rather than text.
a.merge_join_by(b, |i, j| i.cmp(j)),
vec![Both(0, 0), Left(2), Right(3), Left(4), Both(6, 6), Left(1), Right(9)]
);
The advantage of this is that if you just do Ctrl+F and type comm or diff on the page (as opposed to using the rustdoc search feature) you will find it.
(Rust crates often have excellent documentation, except for discoverability when you don't know the term you are looking for. That is not an easy problem, and the fact that rustdoc search doesn't do full text search doesn't help. At least itertools is based on a central trait, so almost everything can be found on a single HTML page. As such Ctrl-F works okay for it. Good luck finding things in crates like regex or clap if you don't know the correct name!)
Ord::cmp
would indeed be a bit better. I just fixed some warnings in the doctests (thanks to a link you posted above) but there is no proper way to use clippy on them yet (https://github.com/rust-lang/rust/issues/56232).
I agree that rustdoc-search is sometimes not as helpful as it could be (especially for a complex crate like clap) but we are quite centralized with the Itertools
trait.
So doc aliases and a comment, fine by me.
@VorpalBlade Done in #966, you don't need to do it yourself.
I recently had the need for comparing sorted iterators to figure out which elements were only in the left, right or both iterators. This is basically the same operation as comm(1) but generalised to iterators.
So I wrote an implementation that works for my purposes. However, this is a side thing in the crate I'm working on, it doesn't fit as an exposed API from it. So I wonder if there is any interest in cleaning up this and going through the effort of making a PR for itertools (where it seems like a good fit).
Below is the basic implementation (excluding tests,
Clone
,Debug
,FusedIterator
and other things that aren't core to the API). I left the doctest in to show how it is used. There are probably ways to improve all of this, but lets see if there is any interest in this at all before I spend time on that.