itertools icon indicating copy to clipboard operation
itertools copied to clipboard

Add `equal() -> Option<Self::Item>` as companion to `all_equal() -> bool`

Open Byron opened this issue 3 years ago • 7 comments

This allows use the equal value or to default it, which otherwise is cumbersome and complex as indicated in this example from nameless:

diff --git a/examples/text-cat.rs b/examples/text-cat.rs
index de1ceea..aac4db5 100644
--- a/examples/text-cat.rs
+++ b/examples/text-cat.rs
@@ -10,12 +10,12 @@ use nameless::{InputTextStream, LazyOutput, MediaType, OutputTextStream};
 /// * `inputs` - Input sources, stdin if none
 #[kommand::main]
 fn main(output: LazyOutput<OutputTextStream>, inputs: Vec<InputTextStream>) -> anyhow::Result<()> {
-    let media_type = match inputs.iter().next() {
-        Some(first) if inputs.iter().map(InputTextStream::media_type).all_equal() => {
-            first.media_type().clone()
-        }
-        _ => MediaType::text(),
-    };
+    let media_type = inputs
+        .iter()
+        .map(InputTextStream::media_type)
+        .equal()
+        .cloned()
+        .unwrap_or_else(MediaType::text);

     let mut output = output.materialize(media_type)?;

Open Questions

  • Is equal() a fitting name? Are there better alternatives?

Byron avatar Jan 04 '22 12:01 Byron

equal to me is the same name as eq, so I find it very odd that it's not returning anything bool-like.

This is an efficient way to do .dedup().exactly_one().ok(), right? So maybe that can provide naming inspiration.

To me, this feels like it wants to return something structurally equivalent to a MinMaxResult: Zero elements, One element, or Multiple elements. Or maybe something like what exactly_one does...

scottmcm avatar Jan 13 '22 19:01 scottmcm

Thanks everyone for helping to improve this PR :)! I have addressed the suggestions of @phimuemue which included changing the name to all_equal_item(), which has advantages for discovering this method.

I understand @scottmcm suggests we could return a Result, but for that I would need guidance as I can't seem to find the connection to exactly_one() and how a special Err value can improve the usability.

Byron avatar Jan 14 '22 00:01 Byron

I think the Option approach, as you have it now, is good, especially with the all_equal_item name -- a 2-state response is consistent with the bool of all_equal.


But to elaborate on the Result thing: think about what you might want to say about what went wrong to a user if they're not all equal.

If the method returned, say Result<Item, Option<(Item, Item)>>, then you could imagine

match r {
    Ok(x) => ... use x ...,
    Err(None) => "There weren't any items",
    Err(Some((a, b))) => format!("There were multiple distinct items, including at least {a} and {b}"),
}

With just a None you can't say something so useful. And it's no more expensive to give out those items, because by the time you'd be able to return a false/None you have to have found the mismatched item.

scottmcm avatar Jan 14 '22 00:01 scottmcm

Thanks for elaborating, now I understand.

Great, thanks everyone!

Byron avatar Jan 14 '22 00:01 Byron

I have undone the previous attempt to reduce duplication as it breaks tests. Given the small amount of code I hope that's acceptable.

Byron avatar Jan 14 '22 13:01 Byron

Oh, I see. empty().all_equal() is true, but empty().all_equal_item() is None.

I see why both of them are that way, but the difference is unfortunate. It makes me second-guess what I previously thought was a great name 🙁

scottmcm avatar Jan 14 '22 13:01 scottmcm

I think the name is perfect, as

  • it's discoverable with the method providing more information having the same prefix.
  • it's not at all surprising that if there is no item, it must return None

I am sure there was a good reason to default all_equal() to true if there is no item at all, and think the problem is rather that both methods lack a way of differentiating the 'empty()case. Doing so would allow the caller to decide whatall_equal()` means if there is no item at all.

Maybe something like all_items_equal() isn't a companion of all_items(), but would rather have a new companion which allows differentiating the empty case? Or should all_items() rather default to false if there is nothing to compare to in the first place? Or should there be simply another variant of this method that returns false then, making it the elusive companion I was talking about?

Seeing all the text above makes me smile a bit as I didn't expect this topic to be as complex as it turned out to be, thanks for luring it out of hiding, it's a conversation definitely worth having :)!

Byron avatar Jan 14 '22 23:01 Byron