Add more functions and tests to Js.Option

[alcarvalho]

I have added what I consider basic functions that should be in every standard library for options. I also created tests for all functions as we didn't have any for these.

I tried running the doc generating script here, but it changes a lot more files than what I was expecting, so I'm leaving this out for now. Let me know how to proceed in this case.

[bobzhang]

this is the same as andThen

[alcarvalho]

Actually andThen is a flatMap. In that case f returns an option in this case f returns just a 'b.

[OvermindDL1]

map is indeed correct here.

andThen is, to be honest, a wtf. Usually I know that function as bind, never seen it named andThen before and that name does not make sense to me... bind though is not much more descriptive really, but bind is the common name for it. flat_map I would like instead of andThen (what is with the camelcase anyway, ocaml is generally snake-case?).

[glennsl]

andThen is used by Elm and Rust, at the very least. And bind is rightly discarded by any new language seeking some actual adoption. I'd be fine with flatMap (and snake_case too) but andThen (and camelCase) is more intuitive to a lot more developers.

[OvermindDL1]

Heh, I don't really get how it is andThening something, but flat_map makes sense based on the words. :-)

A use-case for and_then would be something that is 'temporal', like on a promise or so, but not on an option.

[OvermindDL1]

Or it seems to work just as fine with flat_map while making more grammatical sense since you are not anding anything:

toValidMonth : Int -> Maybe Int
toValidMonth month =
    if month >= 1 && month <= 12 then
        Just month
    else
        Nothing

getFirstMonth : List Int -> Maybe Int
getFirstMonth months =
    head months
    |> Option.flat_map toValidMonth

[OvermindDL1]

Plus this would make a good usecase for filter too! ^.^

let is_valid_month month =
  month >= 1 && month <= 12

let to_valid_month month =
  month |> Option.filter is_valid_month

let head = function
  | [] -> None
  | first :: _ -> Some first

let get_first_month months =
  head months
  |> to_valid_month

So now to_valid_month is not doing the dual-task of both testing a predicate and packing a value (thus the predicate can be used elsewhere now), and piping into functions straight is always better when possible.

This is a side note though, but still, flat_map is still more descriptive than and_then, which just makes no grammatical sense, plus is an actual standard word. ;-)

Could just do both though. :-)

[alcarvalho]

Could just do both though. :-)

I was going to say that... maybe we should implement both the standard library signatures and have aliases that are friendlier to people coming from javascript?

[glennsl]

@OvermindDL1 I think you're missing the point of a somewhat contrived example :) However deliberate, it shouldn't be too hard to imagine other use cases that fit the pattern of do this thing (that returns an option) andThen do this other thing (that also returns an option) with it. It's certainly a more imperative viewpoint, but most devs do actually come from the imperative world. flatMap on the other hand is incomprehensible to anyone who hasn't already memorized its function, although it does have the nice property of being fairly easy to remember once you do learn of it.

[glennsl]

re: implementing both, I'd rather just have flatMap then. Having to memorize two functions is not in any way friendlier to newcomers than having to memorize just one. It's a non-solution that ignores the goal entirely.

Whether or not optimizing for friendliness should be a goal is of course debatable, but this is probably not the right place to have that discussion.

[bobzhang]

name to getWithDefault?

[OvermindDL1]

The stock OCaml Option module (bucklescript really needs to update) uses default for this exact same function:

let default d = function
| None -> d
| Some a -> a

Probably be good to add in the same names at the very least.

[OvermindDL1]

We should probably copy all of these: http://ocaml-lib.sourceforge.net/doc/Option.html

[alcarvalho]

@OvermindDL1 I thought I'd do that, but they said on Discord that this naming was intended to make sense to javascript developers. I questioned on whether we should instead just use the standard functions that you posted as that would make the code more reusable between server and client applications.

[OvermindDL1]

@OvermindDL1 I thought I'd do that, but they said on Discord that this naming was intended to make sense to javascript developers. I questioned on whether we should instead just use the standard functions that you posted as that would make the code more reusable between server and client applications.

Considering I'm planning to have bucklescript-tea have a server-side part soon it better work in native code as well. Even from what little of the horror's I've seen of the 'JS perspective' so far I still do not see how some of their names make any sense, like andThen for a concept as such as in here... >.>

[bobzhang]

is this function common?

[OvermindDL1]

Eh, not really 'common', but it is something I've used before myself, would be nice to include it even if it is not really 'standard'.

[alcarvalho]

I use this all the time in Scala and Swift, to further narrow options. Usually filtering empty strings, for instance. I've seen it in every stdlib of languages that support options.

[OvermindDL1]

I've seen it in every stdlib of languages that support options.

Yet not in the OCaml Option module. ^.^

But still, I'd like it in, it is a pattern I use. :-)

[bobzhang]

cool, lets add filter then.
@OvermindDL1 there is no option module in ocaml stdlib so far as I know..

[bobzhang]

We can argue about naming forever, let's stick to camlCase : ). The reason is existing JS api is camlCase, if we use snake_case, then we will introduce two sets of name conventions

[OvermindDL1]

We can argue about naming forever, let's stick to camlCase : ). The reason is existing JS api is camlCase, if we use snake_case, then we will introduce two sets of name conventions

OCaml already has a convention though. ;-)

Let Reason libraries do things JS'y, OCaml should stay OCaml.

[bobzhang]

OCaml does not really have an official name convention, see unison https://github.com/bcpierce00/unison/blob/master/src/common.ml, it used camlCase.. reply@reply.github.com At: 06/02/17 10:19:00" data-digest="From: reply@reply.github.com At: 06/02/17 10:19:00" style=""> From: reply@reply.github.com At: 06/02/17 10:19:00 To: bucklescript@noreply.github.com Cc: Hongbo Zhang (BLOOMBERG/ 731 LEX), comment@noreply.github.com Subject: Re: [bloomberg/bucklescript] Add more functions and tests to Js.Option (#1669) We can argue about naming forever, let's stick to camlCase : ). The reason is existing JS api is camlCase, if we use snake_case, then we will introduce two sets of name conventions OCaml already has a convention though. ;-) Let Reason libraries do things JS'y, OCaml should stay OCaml. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[OvermindDL1]

In libraries not really I've noticed as well, but in the standard library it is fairly consistent (at least I cannot think of any camelCase off-hand, much as that would go with the OCaml name. ^.^).

[alcarvalho]

Ok, so let's try to summarize the changes here? :)

• Change orValue function to default so it's the same as in the stdlib.
• Keep everything else as is? So not don't add aliases to functions.

[OvermindDL1]

Change orValue function to default so it's the same as in the stdlib.

The Core lib uses value though, it might be the best style to use as the upcoming stdlib changes are modeled heavily on Core. Specifically it defines in Option:

val value : 'a t -> default:'a -> 'a

I really do not like that the named argument is last instead of first, it makes piping it much more difficult...
It also uses these for some of the above as well:

val value_map : 'a t -> default:'b -> f:('a -> 'b) -> 'b
val map2 : 'a t -> 'b t -> f:('a -> 'b -> 'c) -> 'c t
val call : 'a -> f:('a -> unit) t -> unit
val apply : 'a -> f:('a -> 'b) t -> 'b t

Among others: https://ocaml.janestreet.com/ocaml-core/109.12.00/doc/core/Option.html

I could see javascript users liking these names a lot better than the more 'traditional' names as well.

Maybe it would be best to subsume's Core.Option instead? (Maybe other Core modules too?) (Perhaps less labels too, or supply both?)

[alcarvalho]

@OvermindDL1 I think we should follow the stdlib here and if you want to use Core, then you'll be able to, now that they've released the Base library. It should be compatible with bucklescript.

[bobzhang]

@alcarvalho would you sign off your commit (see CONTRIBUTION.md) and I will merge it thanks!
@OvermindDL1 contributions are welcome : )

[alcarvalho]

@bobzhang done! Thanks!