Add exactly_one function #310
Conversation
|
Relevant: rust-lang/rust#55355. |
|
I'd prefer this in Centril's version. And now we know, std doesn't want this method, so we can take it. |
|
(Feel free to copy the contents from my PR if you want) |
|
@bluss Why do we prefer returning nothing in the case of there being more than 1? Since both versions consume the iterator it seems to me we're just needlessly hiding valuable debugging info. |
|
If we simply don't like memory allocation for the error vec then I suppose we could make this take |
|
In all the cases where I needed this I didn't want to get the remaining elements so the allocation shouldn't be pushed on folks imo; If you want to give back the iterator you can do: |
|
We can return an iterator or similar, we already have one for iterator + one item in itertools, PutBack, so that s a nice one to put in the error case |
|
@bluss I've rewritten this to use an iterator in the error case. |
|
Oh right, we need to put back two elements. Either use putbackn or a custom iterator in that case, I don't mind the latter |
|
I think this turned out nicely. :) |
|
@bluss how's this? |
|
putbackn uses a vec internally which puts us back at the original problem, so I went for a custom iterator. |
|
@bluss Just pinging you in case this got lost. Based on your github history it seems the past few days have been very busy for you. |
|
This is good, I'll leave comments inline. I don't always have time. If you look at this repo, one might have to wait months. Pinging is always good. 🙂 |
|
@bluss done, please review |
|
@bluss Believe this is ready to merge now. |
| I: Iterator, | ||
| { | ||
| /// Creates a new `ExactlyOneErr` iterator. | ||
| pub fn new(first_two: (Option<I::Item>, Option<I::Item>), inner: I) -> Self { |
There was a problem hiding this comment.
this should be pub(crate) and not public, but I'll fix it up
|
Sorry, we need to add at test for this iterator before it merges. Then I agree it's ready. Can be a simple quickcheck test. |
|
@bluss is the doc comment test not sufficient? |
|
@Xaeroxe I realize I had thought about tests before and accepted the doctests, so I'm sorry that I didn't remember that. I shouldn't change my mind. But in principle, no, features should have tests. Preferably quickcheck tests! They are very good at finding bugs. I feel strongly that testing documentation is often misunderstood — we test documentation to make sure that the examples we give actually work. We should make understandable examples, and they are there for documentation, not testing. |
|
So since it's annoying to change one's mind, I'll merge since I had already decided some point prior to just let it slide. |
|
bors r+ Thank you! |
|
@bluss well I mean you made a really good case and I agree with you, so I'm happy to add the unit tests if you want. |
Build succeeded |
|
@Xaeroxe ok, that can be a new PR if you want to |
|
Wheels of bors are already turning |
Adds a small function I found uses for personally. Can't be added to the core library because in the error case it allocates a vec and stores data in it.