Skip to content
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

Merged
merged 1 commit into from Sep 13, 2019

Conversation

gasche
Copy link
Member

@gasche gasche commented Jul 25, 2019

Edit: the original name proposed was List.find_some, but I edited the PR title to follow the current naming consensus.

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,
Copy link
Member

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".

Copy link
Contributor

@nojb nojb left a 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.ml Outdated Show resolved Hide resolved
stdlib/list.mli Show resolved Hide resolved
@Armael
Copy link
Member

Armael commented Jul 25, 2019

Note that both Containers and Batteries use List.find_opt as a name for this function.
It might be worth sticking to that name if that's what people are already used to.
Nvm!

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(** [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.

@yallop
Copy link
Member

yallop commented Jul 25, 2019

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 Some v rather than a value for which f returns Some v.

@nojb
Copy link
Contributor

nojb commented Jul 25, 2019

Note that both Containers and Batteries use List.find_opt as a name for this function.
It might be worth sticking to that name if that's what people are already used to.

List.find_opt would surely have a different signature: ('a -> bool) -> 'a list -> 'a option, right?

@nojb
Copy link
Contributor

nojb commented Jul 25, 2019

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 Some v rather than a value for which f returns Some v.

What about List.mapfind or List.map_find?

@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 25, 2019

I agree with @yallop I think find_map (and a correponding filter_map) would be nicer.

EDIT: base uses find_map

@nojb
Copy link
Contributor

nojb commented Jul 25, 2019

I agree with @yallop I think find_map (and a correponding filter_map) would be nicer.

Since we already have List.filter_map, I think this is what makes the most sense.

@alainfrisch
Copy link
Contributor

LGTM, modulo the naming. We already have filter_map, so find_map would look coherent, or perhaps find_map_opt.

@yallop
Copy link
Member

yallop commented Jul 25, 2019

Agreed: I like find_map (or perhaps find_map_opt), for the analogy with the existing filter_map

@nojb
Copy link
Contributor

nojb commented Jul 25, 2019

Agreed: I like find_map (or perhaps find_map_opt), for the analogy with the existing filter_map

I am against using _opt for functions returning options; rather I would use _exn for those raising exceptions; we did it the other way around for existing functions in the stdlib because we did not have an alternative. But for new functions I would rather not use _opt, even if it results in an unfortunate incoherence with existing functions.

Just my 2 cents.

@bluddy
Copy link
Contributor

bluddy commented Jul 25, 2019

But for new functions I would rather not use _opt, even if it results in an unfortunate incoherence with existing functions.

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 _opt convention or _exn convention, you're screwed.

I'd prefer to have an Stdlib.V2 that fixes these things, deprecating the old stdlib.

@nojb
Copy link
Contributor

nojb commented Jul 25, 2019

Much as I would like to achieve this goal, I think it would make the stdlib near unusable.

Maybe. In any case, I will not complain if find_map_opt is chosen; it was just that I find the _opt suffix ugly and I think it is a pity to have ugly names for commonly used functions.

@gasche
Copy link
Member Author

gasche commented Jul 25, 2019

I'm happy with find_map. (Initially I thought of filter_find, in analogy with filter_map, but your analogy is better than my analogy.) Thanks for the comments on the .mli, I will also integrate them.

(I don't think that _opt is necessary here, again in analogy with filter_map.)

@gasche gasche force-pushed the find_some branch 2 times, most recently from 745f57e to de9a357 Compare July 25, 2019 15:20
@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 25, 2019

@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 _opt suffix reads very weirdly if not plain wrong (e.g. In32.of_string_opt should really have been Int32.option_of_string).

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 Stdlib.V2 design are misguided in my opinion and rather preventing us from making progress towards we would all like to have eventually.

@gasche
Copy link
Member Author

gasche commented Jul 25, 2019

Here is the .mli wording that I used:

[find_map f l] applies [f] to the elements of [l] in order,
and returns the first result of the form [Some v], or [None]
if none exist.

(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.)

@gasche
Copy link
Member Author

gasche commented Jul 25, 2019

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 _opt (I don't think we can have a broad consensus on the issue in general; but I feel that we may reach a consensus on this specific function -- we may already have a consensus), and I guess that the discussion on the .mli wording is still ongoing.

Changes Outdated Show resolved Hide resolved
stdlib/list.mli Show resolved Hide resolved
stdlib/list.ml Show resolved Hide resolved
@gasche gasche changed the title List.find_some : ('a -> 'b option) -> 'a list -> 'b option List.map : ('a -> 'b option) -> 'a list -> 'b option Jul 25, 2019
@gasche gasche changed the title List.map : ('a -> 'b option) -> 'a list -> 'b option List.find_map : ('a -> 'b option) -> 'a list -> 'b option Jul 25, 2019
@trefis trefis added the submitter-action-needed This PR is waiting for an action of its submitter. label Aug 16, 2019
Copy link
Contributor

@dbuenzli dbuenzli left a 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.

@gasche gasche force-pushed the find_some branch 2 times, most recently from f30542d to a33f3a3 Compare September 9, 2019 09:01
@gasche
Copy link
Member Author

gasche commented Sep 9, 2019

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.)

@alainfrisch
Copy link
Contributor

LGTM as well, will merge soon if nobody objects. (While reviewing the interface, it occurred to me that it would make sense to move filter_map after filter in list.mli, but this is not really related to this PR.)

@alainfrisch alainfrisch merged commit 5c7c619 into ocaml:trunk Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib submitter-action-needed This PR is waiting for an action of its submitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants