Forbid usage of some complex Option:: methods. #5754
Conversation
| .map_or(lower_bound, |current| current.max(lower_bound)), | ||
| ); | ||
|
|
||
| self.start_timestamp = self.start_timestamp.max(Some(lower_bound)); |
There was a problem hiding this comment.
i know this is correct, but i find relying on Ord of Option to not be something easy to read. If we're moving away from map_or (which i don't think we should ban), i prefer the more verbose "min" variant line 1540, or even better, a .map(max).unwrap_or(val) (you might not like that though)
rational: to me, this operation is "take the larger of the values, or lower_bound if start_timestamp is unset", and this is how the code used to read. With just max, you need some thinking to make sure this is correct, and there isn't really a human sentence to describe what it does. With .unwrap_or(bound).max(bound) we say "take the current value, or the bound if unset, and compare that to the bound", which is the same, but with a stranger wording
There was a problem hiding this comment.
If we're optimizing for Paul's definition of readability, we should rename start_timestamp to start_timestamp_opt and then:
self.start_timestamp_opt = if let Some(start_timestamp) = start_timestamp_opt {
Some(start_timestamp.max(lower_bound))
} else {
Some(lower_bound)
}| # These functions hurt readability (according to Paul) | ||
| "std::option::Option::is_some_and", | ||
| "std::option::Option::is_none_or", | ||
| "std::option::Option::xor", | ||
| # "std::option::Option::and_then", | ||
| # .map(..).unwrap_or(..) or let Some(..) else {..} | ||
| "std::option::Option::map_or", | ||
| # .map(..).unwrap_or_else(..) or let Some(..) else {..} | ||
| "std::option::Option::map_or_else", |
There was a problem hiding this comment.
i don't agree with this list (except probably for xor). i generally like functional-style operation chaining. Can other people weight in on what we should do? cc @guilload
There was a problem hiding this comment.
I don't think we should vote on this issue, because the U.S. western democracies show us that it does not work. Sorry, I'm feeling cheeky this morning.
I also generally like functional-style operation chaining, so I'm biased. However, I do agree that Rust is taking it too far sometimes.
I also don't think we should take this issue too seriously because once the conversation is about personal taste and preference, it's hard to have a productive exchange.
Generally, I'm fine with allowing none of the functional constructs, some of them, or all of them. In the first case, it might make the code more readable for some people. In the second, we try to strike a balance and code might be more enjoyable to write for people like Trinity and me. Finally, in the third case, it's just beneficial for Paul's brain plasticity ;)
As main contributors of Quickwit, I invite you two to have a face to face call to find a middle ground that works for both of you.
There was a problem hiding this comment.
For me they are all hard to read except is_some_and.
Generally map + unwrap_or is very readable to me (and also kinda standardized)
I think this is should not be about personal taste, but readability, e.g. is_some_and is readable to me, but I don't like it.
There was a problem hiding this comment.
okay, so let's replace .map_or() and its variants with .map().unwrap_or() then
fulmicoton-dd
force-pushed
the
forbid_option_complex_methods
branch
from
a69ea8f to
ca6263e
Compare
No description provided.