Add Result module to Belt #2621

mchaver · 2018-03-13T19:19:04Z

Most of these functions are from here
https://gist.github.com/NicolasT/65dad40b203da7c65b4c
I added rights, lefts, arrayLefts and arrayRights.

mchaver · 2018-03-13T20:12:09Z

It's probably breaking tests because I am not sure of all the places I need to reference Belt_Either, but do you think this looks good?

andreypopp · 2018-03-13T21:07:18Z

I'm just starting to use Belt but this looks to me like it breaks few Belt (or OCaml) conventions:

Why not type ('a, 'err) result = Ok of 'a | Error of 'err, OCaml has built-in result type since 4.03 (I think) would be nice to support it in Belt instead of rolling own isomorphic type?
Methods should be all in camelCase in Belt
Not sure if bimap, pure are needed... at least at the start.

mchaver · 2018-03-13T21:46:10Z

@andreypopp
Yeah, I can rename it to Result and fix the camel case. I would like to have bimap, the monadic stuff liek pure, bind, etc. is less important for me.

mchaver · 2018-03-13T22:08:56Z

@andreypopp
It's untested but do you like the naming better?

chenglou · 2018-03-26T22:00:11Z

@mchaver let's get this in. Though is it possible to ship a subset of this first? There are quite a but of bikeshedding opportunity that we can avoid if this is just the same subset as e.g. #2622 for now.

mchaver · 2018-03-26T22:58:43Z

@chenglou sure, I can make it match the same subset of functions as option

mchaver · 2018-03-26T23:38:04Z

@chenglou What is the proper way to get the tests running locally?
I did yarn and yarn test
It runs some of the tests but it can't find Belt_Option

Building ../others/belt.ml
File "/home/james/work/mchaver-bucklescript/jscomp/others/belt.ml", line 244, characters 16-27:
Error: Unbound module Belt_Option
Makefile:290: recipe for target '../others/belt.cmj' failed
make[1]: *** [../others/belt.cmj] Error

chenglou · 2018-03-27T00:12:57Z

Try cd jscomp/others && make depend && make all

mchaver · 2018-03-27T00:52:32Z

make depend seemed to work, but then I got this for make all.

../../lib/bsc.exe  -no-alias-deps -bs-no-version-header -absname -bs-diagnose -bs-no-check-div-by-zero -bs-cross-module-opt -bs-noassertfalse -bs-package-name bs-platform  -nostdlib -bs-package-output commonjs:lib/js -bs-package-output es6:lib/es6  -I ../runtime -I ../stdlib -w +3-40-49 -warn-error A -bin-annot  -c belt_SortArrayString.ml
Building belt_SortArrayString.ml
File "/home/james/work/mchaver-bucklescript/jscomp/others/belt_SortArrayString.ml", line 1:
Error: The implementation belt_SortArrayString.ml
       does not match the interface belt_SortArrayString.cmi:
       The value `diff' is required but not provided
       The value `intersect' is required but not provided
       The value `union' is required but not provided
       The value `binarySearch' is required but not provided
       The value `stableSort' is required but not provided
       The value `stableSortInPlace' is required but not provided
       The value `isSorted' is required but not provided
       The value `strictlySortedLength' is required but not provided
       The type `element' is required but not provided
Makefile:178: recipe for target 'belt_SortArrayString.cmj' failed
make: *** [belt_SortArrayString.cmj] Error 2

chenglou · 2018-03-27T03:58:53Z

Humm... I suggest you stash the changes, do a git clean -xdf, then re-run the stuff at https://github.com/BuckleScript/bucklescript/blob/master/CONTRIBUTING.md#setup

cc @bobzhang

chenglou

Ping bob or me on discord if you can't make the build work. Sorry for the trouble; we're trying to streamline it still

chenglou · 2018-03-27T04:17:35Z

jscomp/others/belt_Result.ml

+let mapWithDefault opt default f = mapWithDefaultU opt default (fun[@bs] x -> f x)
+
+let mapU opt f = match opt with
+  | Ok x -> Some (f x [@bs])


Ok (f x [@bs])

mchaver · 2018-03-27T08:11:39Z

jscomp/others/belt_Result.ml

+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *)
+
+
+type ('a, 'b) t =


Do you like this or should this just point to Js_result.t?

chenglou · 2018-03-27T08:20:50Z

I've fixed your errors inline. Now the build passes!

The interface file is basically like what's from https://github.com/BuckleScript/bucklescript/pull/2622/files. So I think this is good to go. One last question about whether we should alias the type, then that's it. We should redirect folks from Js.Option and Js.Result to here

mchaver · 2018-03-27T08:32:24Z

@chenglou Thanks for helping me clean it up and showing me how to build it. I switched the type to an alias type ('a,'b) t = ('a, 'b) Js_result.t

chenglou · 2018-03-27T23:50:08Z

@bobzhang should we change Js.Result to refer to Belt.Result or vice-versa?

bobzhang

This in general looks good to me

bobzhang · 2018-04-09T02:37:40Z

jscomp/others/belt_Result.mli

+
+type ('a,'b) t = ('a, 'b) Js_result.t = Ok of 'a | Error of 'b
+
+val getExn : ('a, 'b) t -> 'a


should getExn contain some information about Error ?

chenglou · 2018-04-09T02:44:56Z

should we change Js.Result to refer to Belt.Result or vice-versa?

I think we should flip it and make Js.Result refer to Belt.Result, then put deprecation notices on Js.Result

mchaver · 2018-04-27T04:02:51Z

@chenglou
I think I might be missing something. Do dependencies need to be reordered so js_result can find Belt.Result?

File "/usr/local/lib/node_modules/bs-platform/jscomp/others/js_result.mli", line 25, characters 18-40:
Error: Unbound module Belt_Result
Makefile:176: recipe for target 'js_result.cmi' failed
make[2]: *** [js_result.cmi] Error 2
make[2]: *** Waiting for unfinished jobs....
Building belt_SortArrayString.mli
Building js_mapperRt.mli
Building belt_SortArrayInt.mli
Building belt_Id.mli
Building belt_Array.mli
Building belt_List.mli
make[2]: Leaving directory '/usr/local/lib/node_modules/bs-platform/jscomp/others'
Makefile:14: recipe for target 'libs' failed
make[1]: *** [libs] Error 2
make[1]: Leaving directory '/usr/local/lib/node_modules/bs-platform'
Makefile:9: recipe for target 'world' failed
make: *** [world] Error 2
child_process.js:516
    throw err;

mchaver · 2018-05-10T07:46:48Z

@chenglou @bobzhang
Could you guys take another look at this?

chenglou · 2018-05-18T00:00:50Z

@mchaver thanks for the PR! =D And sorry for the back-and-forth, but it looks great now.

mchaver added 3 commits March 14, 2018 03:10

Add Belt.Either

1284c50

Fix notes

2d54f83

Update function names

3f52573

Move to result

130db55

mchaver added 2 commits March 14, 2018 06:17

Rename things

e0d98bf

Update Makefile

db3f192

mchaver changed the title ~~Add Either to Belt~~ Add Result module to Belt Mar 14, 2018

Match functions from Belt_Option

9b55b6e

chenglou reviewed Mar 27, 2018

View reviewed changes

chenglou added 3 commits March 27, 2018 00:37

Update belt_Result.ml

f86537c

Update belt_Result.ml

94937b6

Update Makefile

b3e40ee

mchaver commented Mar 27, 2018

View reviewed changes

Update .depend

2a77fbf

Use Js_result.t

df16b63

Add constructors

139e81e

bobzhang reviewed Apr 9, 2018

View reviewed changes

mchaver added 2 commits April 27, 2018 11:58

Deprecate Js.Result

fae05c5

add belt_result.js

00c5302

mchaver added 2 commits May 10, 2018 15:33

Merged master

3f48d05

revert js_result, use deprecation only

0033bf6

bobzhang merged commit d8f50d2 into rescript-lang:master May 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Result module to Belt #2621

Add Result module to Belt #2621

mchaver commented Mar 13, 2018

mchaver commented Mar 13, 2018

andreypopp commented Mar 13, 2018

mchaver commented Mar 13, 2018

mchaver commented Mar 13, 2018

chenglou commented Mar 26, 2018

mchaver commented Mar 26, 2018 •

edited

mchaver commented Mar 26, 2018

chenglou commented Mar 27, 2018

mchaver commented Mar 27, 2018

chenglou commented Mar 27, 2018

chenglou left a comment

chenglou Mar 27, 2018

mchaver Mar 27, 2018

chenglou commented Mar 27, 2018 •

edited

mchaver commented Mar 27, 2018

chenglou commented Mar 27, 2018

bobzhang left a comment

bobzhang Apr 9, 2018

chenglou commented Apr 9, 2018

mchaver commented Apr 27, 2018

mchaver commented May 10, 2018

chenglou commented May 18, 2018 •

edited

		* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *)


		type ('a, 'b) t =


		type ('a,'b) t = ('a, 'b) Js_result.t = Ok of 'a \| Error of 'b

		val getExn : ('a, 'b) t -> 'a

Add Result module to Belt #2621

Add Result module to Belt #2621

Conversation

mchaver commented Mar 13, 2018

mchaver commented Mar 13, 2018

andreypopp commented Mar 13, 2018

mchaver commented Mar 13, 2018

mchaver commented Mar 13, 2018

chenglou commented Mar 26, 2018

mchaver commented Mar 26, 2018 • edited

mchaver commented Mar 26, 2018

chenglou commented Mar 27, 2018

mchaver commented Mar 27, 2018

chenglou commented Mar 27, 2018

chenglou left a comment

Choose a reason for hiding this comment

chenglou Mar 27, 2018

Choose a reason for hiding this comment

mchaver Mar 27, 2018

Choose a reason for hiding this comment

chenglou commented Mar 27, 2018 • edited

mchaver commented Mar 27, 2018

chenglou commented Mar 27, 2018

bobzhang left a comment

Choose a reason for hiding this comment

bobzhang Apr 9, 2018

Choose a reason for hiding this comment

chenglou commented Apr 9, 2018

mchaver commented Apr 27, 2018

mchaver commented May 10, 2018

chenglou commented May 18, 2018 • edited

mchaver commented Mar 26, 2018 •

edited

chenglou commented Mar 27, 2018 •

edited

chenglou commented May 18, 2018 •

edited