upgrade to new bs-platofrm 4.0.7 #10

jimmyhuco · 2018-11-14T05:09:00Z

Thank you for your great cookbook. I have learned so much.

But some bs- libs are absolutely outdated. Likebs-revamp and bs-node.

And I try to improve some code for better demonstration. I hope it's useful.

So I rewrote some code in the book.

BTW: I rewrote all the code list in examples in Reasonml on my example repo

glennsl

Thanks! The removal of bs-node is fine, but I'd prefer to keep the examples for bs-revamp. Most of the other fixes are good, but I have a few questions and comments below.

glennsl · 2018-11-14T13:48:44Z

README.md

@@ -182,8 +183,8 @@ let () =
 Use [Random module](http://caml.inria.fr/pub/docs/manual-ocaml/libref/Random.html) to generate random numbers

 ```ml
-let () =
-  Js.log (Random.int 5)
+let () = Random.init (int_of_float (Js.Date.now ()))


It would be nice if this line was explained, since it isn't very obvious what the purpose of all these functions are

glennsl · 2018-11-14T13:49:32Z

README.md

-let () =
-  Js.log (Random.int 5)
+let () = Random.init (int_of_float (Js.Date.now ()))
+let () = (Random.int 5) |> Js.log


The parentheses here are superfluous when using the pipe operator

glennsl · 2018-11-14T13:51:55Z

README.md

 #### Dasherize camelCased identifiers inside string literals using Regular Expression

-Uses [bs-revamp](https://github.com/glennsl/bs-revamp)


I would rather see a PR update bs-revamp than have the example removed. I don't think much would need to be changed.

The addition of an example that does not use bs-revamp is great though.

I agree. bs-revamp just needs some love.

glennsl · 2018-11-14T13:53:47Z

README.md

@@ -349,7 +351,7 @@ let () =

 ###### Hashtbl

-Mutable, string key type, cross-platform


Why is this removed? Do you not think it's a significant property to mention?

Oh, sorry. It's werid in my machine yesterday. Hashtbl didn't work.

glennsl · 2018-11-14T13:56:34Z

README.md

+  HashMap.String.set painIndexMap "bumble bee" 2.0
+
+let () =
+  painIndexMap |. HashMap.String.forEach (fun k  -> fun v  -> Js.log {j|key:$k, val:$v|j})


A Belt.HashMap example is great, but perhaps it should show the generalized HashMap since it's more widely applicable?

I'm also not a fan of using the fast pipe since it's mostly just confusing. Could you rewrite it without that?

It's ok. but why do you think fast pipe is confusing?

In javascript world, many apis are often attached to objects, and often chainable. data is the first argument in function.

There's already a well established pipe operator, not just in OCaml/Reason but in many other languages as well. The fast pipe isn't even language-specific but platform-specific. It won't work outside BuckleScript, which makes it unusable with anything written to target anything other than BuckleScript. But even within BuckleScript it's both confusing and painful to have to remember which convention and pipe operator you have to use with a given function, especially if you have to juggle both in the same "chain" of function calls. And for newcomers, which is the intended audience here, unfamiliarity makes this confusion much worse.

Of course it's really the bad design of Belt itself that's the root problem here and the fast pipe is mostly a workaround to make serious use more convenient, but we can't do much about that other than to not show it, which I don't think is very honest since it's still a real alternative. But we can at least avoid using cryptic operators that's highly likely to be unfamiliar and confusing to newcomers when it's largely unnecessary to demonstrate it.

glennsl · 2018-11-14T14:01:31Z

README.md

@@ -559,6 +584,8 @@ let person = [%obj {
  };
  age = 32
 }]
+
+let _ = Js.log (## (## person name) first)


This isn't valid OCaml. Looks like you wrote it in Reason and converted it in the Try Reason playground which doesn't handle ## properly. It should be let _ = Js.log (person##name##first).

I'm not sure this line should be there at all though, since the example should illustrate object creation. not object access, and could be confusing.

You are right. It should be removed.

HAHA. You are a wise man. I am an newbie. I use refmt --parse ml --print re --interface false to convert Ocaml syntaxt to Reasonml. I have to lookup Ocaml docs when I meet problems.

In fact, I am very confused. why create a new syntax , named Reasonml? Just for marketing?

Mostly to make it more familiar and approachable to developers already familiar with JavaScript or other Algol-style languages, I think, which is a pretty sizable share of all developers.

But it also fixes a many inconsistencies and general weirdness with OCaml. I prefer Reason, even if it does have its issues too.

glennsl · 2018-11-17T23:02:55Z

README.md

+  (code |>
+     Js.String.unsafeReplaceBy1
+        [%re
+           "/\"([^\"]*)\"/g"]


This formatting is a bit weird. I don't think the line break is particularly necessary here.

glennsl · 2018-11-17T23:06:34Z

README.md

+  Id.MakeHashable(struct
+                    type t = string
+                    let hash (x : t) = String.length x
+                    let eq (a : t) (b : t) = a = b


Are all these type annotations really needed? It should be constrained by the signature MakeHashable expect, shouldn't it? Also, with partial application you should be able to just do let hash = String.length and let eq = (=).

glennsl · 2018-11-17T23:13:25Z

README.md

@@ -452,7 +517,7 @@ let _ : unit Js.Promise.t =
 (* Chaining *)
 let _ : unit Js.Promise.t =
  Js.Promise.then_
-  	(fun value -> Js.Promise.resolve (Js.log value))
+    (fun value -> Js.Promise.resolve (Js.log value))


This isn't your doing, but I just noticed how weird it is to resolve the return value of Js.log. And being more explicit with Js.log value; Js.Promise.resolve () isn't much more verbose either. What do you think?

Both ok. Js.Promise.resolve (Js.log value) is less code. I think everyone knows the return value from Js.log is unit.

glennsl · 2018-11-17T23:14:33Z

README.md

+
+let () =
+  HashMap.forEach painIndexMap
+    (fun k  -> fun v  -> Js.log {j|key:$k, val:$v|j})


The explicit currying and double whitespace formatting is a bit weird. I assume it's because it's auto-translated from Reason?

glennsl · 2018-11-17T23:17:29Z

README.md

-let () =
-  Js.log (Random.int 5)
+(* To make sure you have a different seed when your program runs each time *)
+let () = Js.Date.now () |> int_of_float |> Random.init


I think putting Random.init first here would focus more on the purpose of this invocation:

let () = Random.init (Js.Date.now () |> int_of_float)

jimmyhuco · 2018-11-18T11:46:13Z

Thank you for your careful review. Some issues are made by convert tools because I am not unfamiliar with Ocaml. I think the pull request is not urgent. So I plan to spend many days to fill up some Ocaml knowledge. After that, I will come back.

In the past, I wrote some Haskell/Purescript Codes. I think the bucklescript is on the right way for javascript ecosystem. More important and interesting it gives me a chance to meet Ocaml. And you are the catalyst. Thank you!

glennsl · 2018-11-18T13:13:54Z

Cool, thanks!

Just out of curiosity, why do you think you'll prefer BuckleScript over Purescript? I plan to explore Purescript, and in particular Halogen, and coming from BuckleScript I'm very curious about the perspective coming from the other direction.

jimmyhuco · 2018-11-18T14:19:04Z

Cool.

I have a feeling that another post about why x over y will be generated. But I think it's not proper to discuss in this pull request. I am also inquiring about your choice. I look forward to your reply. Drop me an email: [email protected]. You can share some points first. 😆

upgrade to new bs-platofrm

32584be

glennsl reviewed Nov 14, 2018

View reviewed changes

add or change some code after the review

178e6f6

glennsl reviewed Nov 17, 2018

View reviewed changes

handwrite ocaml

2c355b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade to new bs-platofrm 4.0.7 #10

upgrade to new bs-platofrm 4.0.7 #10

jimmyhuco commented Nov 14, 2018

glennsl left a comment

glennsl Nov 14, 2018

glennsl Nov 14, 2018 •

edited

Loading

glennsl Nov 14, 2018

jimmyhuco Nov 15, 2018

glennsl Nov 14, 2018

jimmyhuco Nov 15, 2018

glennsl Nov 14, 2018

jimmyhuco Nov 15, 2018

jimmyhuco Nov 15, 2018

glennsl Nov 18, 2018

glennsl Nov 14, 2018

jimmyhuco Nov 15, 2018

glennsl Nov 18, 2018

glennsl Nov 17, 2018

glennsl Nov 17, 2018

glennsl Nov 17, 2018

jimmyhuco Nov 24, 2018

glennsl Nov 17, 2018

glennsl Nov 17, 2018

jimmyhuco commented Nov 18, 2018

glennsl commented Nov 18, 2018

jimmyhuco commented Nov 18, 2018

		#### Dasherize camelCased identifiers inside string literals using Regular Expression

		Uses [bs-revamp](https://github.com/glennsl/bs-revamp)

		@@ -349,7 +351,7 @@ let () =

		###### Hashtbl

		Mutable, string key type, cross-platform

upgrade to new bs-platofrm 4.0.7 #10

Are you sure you want to change the base?

upgrade to new bs-platofrm 4.0.7 #10

Conversation

jimmyhuco commented Nov 14, 2018

glennsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl Nov 14, 2018 • edited Loading

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

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

jimmyhuco commented Nov 18, 2018

glennsl commented Nov 18, 2018

jimmyhuco commented Nov 18, 2018

glennsl Nov 14, 2018 •

edited

Loading