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
List.find_map : ('a -> 'b option) -> 'a list -> 'b option #8832
Conversation
stdlib/list.mli
Outdated
@@ -230,6 +230,15 @@ val find_opt: ('a -> bool) -> 'a list -> 'a option | |||
satisfies [p] in the list [l]. | |||
@since 4.05 *) | |||
|
|||
val find_some: ('a -> 'b option) -> 'a list -> 'b option | |||
(** [find_some f l] applies [f] to every element of [l] in turn, |
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.
I prefer "each element" to "every element" here, since the traversal stops once f
"succeeds".
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.
LGTM (modulo trivial remarks). Thanks for your new-found zest for stdlib additions!
|
stdlib/list.mli
Outdated
@@ -230,6 +230,15 @@ val find_opt: ('a -> bool) -> 'a list -> 'a option | |||
satisfies [p] in the list [l]. | |||
@since 4.05 *) | |||
|
|||
val find_some: ('a -> 'b option) -> 'a list -> 'b option | |||
(** [find_some f l] applies [f] to every element of [l] in turn, |
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.
(** [find_some f l] applies [f] to every element of [l] in turn, | |
(** [find_some f l] applies [f] successively to elements of [l], |
might remove the wrong impression that the function is indeed applied to all elements.
This is a useful function to have, but I'm not very keen on the name, which suggests (to me) that the function is looking for a value of the form |
|
What about |
Since we already have |
LGTM, modulo the naming. We already have |
Agreed: I like |
I am against using Just my 2 cents. |
Much as I would like to achieve this goal, I think it would make the stdlib near unusable. You have to be able to immediately think of the name of the function you want. If you can't even remember if it uses the I'd prefer to have an Stdlib.V2 that fixes these things, deprecating the old stdlib. |
Maybe. In any case, I will not complain if |
I'm happy with (I don't think that |
745f57e
to
de9a357
Compare
@bluddy Sigh. We are constantly having the same discussions about the way we should move on forward with some of the historical decision of the stdlib. I think it is much better if we gradually lead towards what we would like to have rather than continue to enshrine weird past decisions. The In these days of wonderful IDE completion I don't think your usability point is one and it's certainly not one as far as code reading is which as usual what should be favored. The stdlib is already largely inconsistent and will likely ever be. People believing we'll be saved by some kind of |
Here is the
(Personally I think it's obvious that the function will stop at the first suitable element, so I'm not too worried about people reading the documentation the wrong way, but I hope that this reformulation will work for @yallop and @alainfrisch.) |
I think that stdlib additions (at least, mine) should be consensus-based rather than passing a single approval. If I understand correctly, the current status of the PR has support from @nojb, @yallop, @dbuenzli and @alainfrisch (please complain if you were misrepresented). No specific concern with the new version has been made explicit, although there is discussion on |
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.
It seems little is needed to get this merged (see @nojb's comment). I again won't mention ListLabels
because it should really die.
A nice touch to the PR would be to add a commit that gets rid of Misc.Stdlib.List.find_map
throughout the compiler codebase.
f30542d
to
a33f3a3
Compare
I rebased the PR and made sure @nojb's comments were addressed. (I won't merge myself; I think that stdlib additions would deserve extra carefulness in getting consensus and approval, and I don't want to speed up the process artificially.) |
LGTM as well, will merge soon if nobody objects. (While reviewing the interface, it occurred to me that it would make sense to move |
Edit: the original name proposed was
List.find_some
, but I edited the PR title to follow the current naming consensus.