Add option module to Belt #2622

arnarthor · 2018-03-14T00:21:31Z

This adds the following utility functions for the option data type to belt.

val getExn : 'a option -> 'a
val mapWithDefaultU : 'a option -> 'b -> ('a -> 'b [@bs]) -> 'b
val mapWithDefault : 'a option -> 'b -> ('a -> 'b) -> 'b
val mapU : 'a option -> ('a -> 'b [@bs]) -> 'b option
val map : 'a option -> ('a -> 'b) -> 'b option
val flatMapU : 'a option -> ('a -> 'b option [@bs]) -> 'b option
val flatMap : 'a option -> ('a -> 'b option) -> 'b option
val getWithDefault : 'a option -> 'a -> 'a
val isSome : 'a option -> bool
val isNone : 'a option -> bool
val eqU : 'a option -> 'b option -> ('a -> 'b -> bool [@bs]) -> bool
val eq : 'a option -> 'b option -> ('a -> 'b -> bool) -> bool
val cmpU : 'a option -> 'b option -> ('a -> 'b -> int [@bs]) -> int
val cmp : 'a option -> 'b option -> ('a -> 'b -> int) -> int

chenglou

Thanks! Left some comments

chenglou · 2018-03-14T03:25:44Z

jscomp/others/belt_Option.ml

@@ -0,0 +1,27 @@
+let getExn = function
+  | Some x -> x
+  | None -> assert false


I believe @bobzhang uses [%assert "getExn"] for some reason

chenglou · 2018-03-14T03:26:08Z

jscomp/others/belt_Option.ml

+  | Some x -> x
+  | None -> assert false
+
+let fold opt default f = match opt with


The name will need some thinking

People from the JS world probably know this as reduce or something along those lines at least for Lists/Arrays. We use fold internally because we're used to it from Scala/Haskell.

chenglou · 2018-03-14T03:26:38Z

jscomp/others/belt_Option.ml

+  | Some x -> f x
+  | None -> default
+
+let map opt f = match opt with


I think we'd also expose some mapU like other modules for consistency. But those can wait after this PR

chenglou · 2018-03-14T03:26:51Z

jscomp/others/belt_Option.ml

+  | Some x -> Some (f x)
+  | None -> None
+
+let flatMap opt f = match opt with


Same with fold regarding naming

chenglou · 2018-03-14T03:28:02Z

jscomp/others/belt_Option.ml

+  | Some x -> x
+  | None -> default
+
+let exists = function


Belt.List uses has. cc @bobzhang another thing we should unify around. has vs contains vs exists. First one is easier for non-english speakers

Actually, not sure why Belt.List.has exists

chenglou · 2018-03-14T03:28:14Z

jscomp/others/belt_Option.ml

+  | Some _ -> true
+  | None -> false
+
+let empty = function


This should be isEmpty

arnarthor · 2018-03-14T10:24:58Z

I've addressed most of your comments @chenglou. I haven't changed the names for fold and flatMap.

Added uncurried versions of all the functions where that applies.

bobzhang · 2018-03-14T12:03:24Z

jscomp/others/belt_Option.mli

+val flatMapU : 'a option -> ('a -> 'b option [@bs]) -> 'b option
+val flatMap : 'a option -> ('a -> 'b option) -> 'b option
+val getOrElse : 'a option -> 'a -> 'a
+val has : 'a option -> bool


has is redundant?

bobzhang · 2018-03-14T12:03:49Z

jscomp/others/belt_Option.mli

+val map : 'a option -> ('a -> 'b) -> 'b option
+val flatMapU : 'a option -> ('a -> 'b option [@bs]) -> 'b option
+val flatMap : 'a option -> ('a -> 'b option) -> 'b option
+val getOrElse : 'a option -> 'a -> 'a


getWithDefault?

bobzhang · 2018-03-14T12:08:02Z

jscomp/others/belt_Option.mli

+val mapU : 'a option -> ('a -> 'b [@bs]) -> 'b option
+val map : 'a option -> ('a -> 'b) -> 'b option
+val flatMapU : 'a option -> ('a -> 'b option [@bs]) -> 'b option
+val flatMap : 'a option -> ('a -> 'b option) -> 'b option


bind or andThen?

Flatmap. It makes much more sense when you need to explain it to newcommers

bobzhang · 2018-03-14T12:09:11Z

jscomp/others/belt_Option.mli

+
+val getExn : 'a option -> 'a
+val foldU : 'a option -> 'b -> ('a -> 'b [@bs]) -> 'b
+val fold : 'a option -> 'b -> ('a -> 'b) -> 'b


is fold common enough to be included?

We at least use it pretty often internally when dealing with a CLI tool we're writing. It's nice to have it to set defaults for missing arguments

this name does not sound meaningful to me, mapWithDefault looks better?

bobzhang · 2018-03-14T12:15:17Z

jscomp/others/belt_Option.mli

+val flatMap : 'a option -> ('a -> 'b option) -> 'b option
+val getOrElse : 'a option -> 'a -> 'a
+val has : 'a option -> bool
+val isEmpty : 'a option -> bool


I kinda feel x = None looks shorter than isEmpty and more efficient.

Some missed functionalities, eq, cmp?

mchaver · 2018-03-14T14:56:33Z

@arnarthor @bobzhang How about a catSomes function

let catSomes (type a) (mas: (a option) list) :a list =
  List.fold_left(fun acc x ->
      match x with
      | None -> acc
      | Some a -> List.append acc [a]
    ) [] mas

and an array version

let arrayCatSomes (type a) (mas: (a option) array) :a array =
  Array.fold_left(fun acc x ->
      match x with
      | None -> acc
      | Some a -> Array.append acc [a]
    ) [||] mas

arnarthor · 2018-03-14T15:53:30Z

I don't really see a usecase for it to be honest. This is just a specialized version of something like keep/filter for both Lists and Arrays right?

mchaver · 2018-03-14T15:58:38Z

@arnarthor It's useful if you have collections of computations that can potentially fail, then you get the successful ones. It's a common function in the stdlibs of other functional programming languages like Haskell, Scala, Purescript, etc. catMaybes in those languages.

utkarshkukreti · 2018-03-14T16:18:08Z

jscomp/others/belt_Option.ml

+  | None -> default
+
+let has = function
+  | Some _ -> true


How about naming these isSome and isNone like Rust? I think they convey what the function actually does better than has/isEmpty.

agreed. In addition to isSome isNone:

Option.unwrap(Some(A)) vs Option.get(Some(A))
Option.unwrapOr(Some(A), B) vs Option.getOrElse(Some(A), B) vs Option.getWithDefault(Some(A), B)

isSome and isNone are nice, but @bobzhang pointed out that x == None is more performant so they actually might not be needed.

I'd say that we should keep getExn and getWithDefault as is for consistency with other Belt modules which have get, getExn, getBy etc.

I agree with suffix consistency there but Option.get doesn't communicate to me that we are unwrapping / moving / extracting the value contained in the Option. unwrapExn unwrapWithDefault imo are more clear, but I don't feel super strongly about it

arnarthor · 2018-03-14T20:57:51Z

@bobzhang I've added cmp and eq functions. I haven't renamed flatMap yet since we there isn't really a clear decisions on what should be used instead. I personally feel like flatMap is the most descriptive in this case, but I really don't mind if it's flatMap or bind. I don't like andThen in this case since it's not really descriptive for options.

I also kept has and isEmpty there but renamed them to isSome and isNone. I know they're redundant since you could use x === None but I feel like having these verbose functions could help newcomers read through code.

Anyways feel free to leave some more comments if you would like me to change something 👍

bobzhang · 2018-03-15T04:43:58Z

Lets see if we can make it for release, see my comments about the naming of fold, is fold a good name for that function

chenglou · 2018-03-15T05:59:25Z

Yeah I think mapWithDefault is a better name for fold too

cullophid · 2018-03-15T06:30:22Z

Since List already has map and flatten it would kinda make sense to call the combination flatMap...

arnarthor · 2018-03-15T10:11:11Z

Done @bobzhang. Think it's ready for merge 👍

arnarthor · 2018-03-15T10:34:14Z

I can't run a rebuild on travis, the instance stalled which caused the error.

chenglou · 2018-03-15T10:35:30Z

Restarted for you. Related: #2564

chenglou · 2018-03-16T06:18:50Z

I'm iffy on the name flatMap/bind/andThen, since it causes so much drama. Maybe we should leave it out for one version?

arnarthor added 2 commits March 14, 2018 00:19

Add option module to belt

e6ecf93

Fix indent in makefile

6ed6ca2

chenglou reviewed Mar 14, 2018

View reviewed changes

arnarthor added 6 commits March 14, 2018 10:15

belt_Option assert false -> [%assert "getExn"]

4976e58

belt_Option empty -> isEmpty

8e2f870

belt_Option exists -> has

781ad87

belt_Option add uncurried versions of map/fold/flatMap

653c87c

Merge branch 'master' into master

c9e3bff

Add license to belt_Option module

a77fcbb

bobzhang reviewed Mar 14, 2018

View reviewed changes

utkarshkukreti reviewed Mar 14, 2018

View reviewed changes

arnarthor added 2 commits March 14, 2018 20:50

Rename getOrElse -> getWithDefault, has -> isSome, isEmpty -> isNone

fa45dd9

Add eq and cmp functions

b03196a

bobzhang added the PRIORITY:HIGH label Mar 15, 2018

fold -> mapWithDefault

9452c2a

bobzhang merged commit 799b561 into rescript-lang:master Mar 16, 2018

chenglou mentioned this pull request Mar 26, 2018

Add Result module to Belt #2621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option module to Belt #2622

Add option module to Belt #2622

arnarthor commented Mar 14, 2018 •

edited

chenglou left a comment

chenglou Mar 14, 2018

chenglou Mar 14, 2018

arnarthor Mar 14, 2018

chenglou Mar 14, 2018

chenglou Mar 14, 2018

chenglou Mar 14, 2018

chenglou Mar 14, 2018

chenglou Mar 14, 2018

arnarthor commented Mar 14, 2018

bobzhang Mar 14, 2018

bobzhang Mar 14, 2018

bobzhang Mar 14, 2018

cullophid Mar 14, 2018

bobzhang Mar 14, 2018

arnarthor Mar 14, 2018

bobzhang Mar 15, 2018

bobzhang Mar 14, 2018

mchaver commented Mar 14, 2018 •

edited

arnarthor commented Mar 14, 2018

mchaver commented Mar 14, 2018

utkarshkukreti Mar 14, 2018

brentvatne Mar 14, 2018

arnarthor Mar 14, 2018

brentvatne Mar 14, 2018

arnarthor commented Mar 14, 2018

bobzhang commented Mar 15, 2018

chenglou commented Mar 15, 2018

cullophid commented Mar 15, 2018

arnarthor commented Mar 15, 2018

arnarthor commented Mar 15, 2018

chenglou commented Mar 15, 2018

chenglou commented Mar 16, 2018

Add option module to Belt #2622

Add option module to Belt #2622

Conversation

arnarthor commented Mar 14, 2018 • edited

chenglou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnarthor commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchaver commented Mar 14, 2018 • edited

arnarthor commented Mar 14, 2018

mchaver commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnarthor commented Mar 14, 2018

bobzhang commented Mar 15, 2018

chenglou commented Mar 15, 2018

cullophid commented Mar 15, 2018

arnarthor commented Mar 15, 2018

arnarthor commented Mar 15, 2018

chenglou commented Mar 15, 2018

chenglou commented Mar 16, 2018

arnarthor commented Mar 14, 2018 •

edited

mchaver commented Mar 14, 2018 •

edited