Additions to js_string and js_array #1203

glennsl · 2017-02-12T10:46:50Z

This more or less brings js_array and js_string to es2015 compliance.

It has the following breaking changes:

Array.slice_copy has been renamed Array.sliceCopy
Array.slice_start has been renamed Array.sliceFrom
Array.lastIndexOf has been renamed Array.lastIndexOfFrom
Array.lastIndexOf_start has been renamed Array.lastIndexOf
String.concat has been renamed String.concatMany
A new non-spliced function has taken the place of the old String.concat

glennsl · 2017-02-12T12:08:07Z

jscomp/others/js_array.ml

+(** https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push *)
+external pop : 'a array -> 'a Js.undefined = "" [@@bs.send]
+external push : 'a array  -> 'a -> int = "" [@@bs.send]
+external pushMany : 'a array  -> 'a array -> int = "push" [@@bs.send] [@@bs.splice]


I'm thinking these should have pipe form too, but was hesitant to make the breaking change,

bobzhang · 2017-02-12T15:26:15Z

hi @glennsl , are you aware of [@@ocaml.deprecated "xx"] maybe you can keep the old definition and add new one, so we can remove the deprecated in the next version?

glennsl · 2017-02-12T20:12:38Z

I was not, but that's cool! I've added back the renamed functions with deprecation warnings, but this still leaves String.concat as a breaking change, and push/pop would be breaking if made into pipe form too.

glennsl · 2017-02-12T20:22:53Z

Just noticed the lastIndexOf and lastIndexOfFrom signatures were wrong, so that's still a breaking change.

bobzhang · 2017-02-13T16:32:47Z

jscomp/others/js_array.ml

-external toLocaleString : unit -> string  = "" [@@bs.send.pipe: 'a t as 'this]
-external concat : 'this -> 'this  = "" [@@bs.send.pipe: 'a t as 'this]
-external append : 'a -> 'this = "concat" [@@bs.send.pipe: 'a t as 'this]
+external isArray : 'a -> Js.boolean = "Array.isArray" [@@bs.val]


This is an es6 feature, shall we document it clearly?

bobzhang · 2017-02-13T16:33:56Z

jscomp/others/js_array.ml

+*)
+external copyWithin : int -> 'this = "" [@@bs.send.pipe: 'a t as 'this]
+external copyWithinFrom : int -> int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this]
+external copyWithinFromTo : int -> int -> int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this]


add label argument?

bobzhang · 2017-02-13T16:35:26Z

jscomp/others/js_string.ml

 * You should have received a copy of the GNU Lesser General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *)
 (* Copyright (C) 2015-2016 Bloomberg Finance L.P.
- * 
+ *


we have two copy right headers here again..

bobzhang · 2017-02-13T16:36:14Z

jscomp/test/js_string_test.js

@@ -29,7 +29,7 @@ function eq(loc, _z) {

 eq('File "js_string_test.ml", line 13, characters 5-12', /* tuple */[
      "012",
-      "0".concat("1", "2")
+      "0".splice("1", "2")


this seems fishy

Sorry, that's been fixed, just missed commiting the generated js. I'll add more tests to avoid issues like this.

bobzhang · 2017-02-13T16:38:06Z

This looks good to me in general, but it would be nice to have some docs/tests for each function

glennsl · 2017-02-14T08:39:31Z

Ok, I've now squashed some more bugs, tweaked the API, added yet more functions and tests for everything. Once you give the OK I'll squash the commits and add the sign-off.

bobzhang

Note I did not finish my review yet, would you do a rebase first, since lots of changes don't belong to this revision, thanks

bobzhang · 2017-02-14T13:54:06Z

.vscode/c_cpp_properties.json

+    "configurations": [
+        {
+            "name": "Mac",
+            "includePath": ["/usr/include", "/Users/hzhang295/.opam/4.02.3+local-git-master/lib/ocaml/caml"],


ah, sorry, I should not have it checked in, would you do me a favor to remove it

bobzhang · 2017-02-14T13:56:00Z

jscomp/others/js_array.ml

-external slice_copy : unit -> 'this = "slice"[@@bs.send.pipe: 'a t as 'this]
-external slice_start : int -> 'this = "slice"[@@bs.send.pipe: 'a t as 'this]
+(* TODO: Not available in Node V4  *)
+external includes : 'a -> Js.boolean = "" [@@bs.send.pipe: 'a t as 'this] (* es2016 *)


better to have such style comments when they are important
(** ES2016 *)

bobzhang · 2017-02-14T13:57:34Z

jscomp/others/js_array.ml

+type 'a array_like
+type 'a array_iter = 'a array_like (* don't think this is very useful to implement wihtout language support *)
+
+external make : int -> unit Js.undefined array = "Array" [@@bs.new]


do we really need introduce function, I think what we have in OCaml stdlib is better `val make : int -> 'a -> 'a array'
This is not a safe function, we can defer its inclusion

I included it for the ability to do things like make 10 |> mapi (fun _ i -> i), but I agree Array.make is better.

bobzhang · 2017-02-14T13:59:01Z

jscomp/others/js_array.ml

+external make : int -> unit Js.undefined array = "Array" [@@bs.new]
+
+external from : 'a array_like -> 'b array = "Array.from" [@@bs.val] (* es2015 *)
+external unsafeFrom : 'a -> 'b array = "Array.from" [@@bs.val] (* es2015 *)


this looks weird, I can not tell its meaning by reading its types, note that JS API is very rich and polymorphic, we don't need expose all of them to our users though

bobzhang · 2017-02-14T13:59:46Z

jscomp/others/js_array.ml

+external from : 'a array_like -> 'b array = "Array.from" [@@bs.val] (* es2015 *)
+external unsafeFrom : 'a -> 'b array = "Array.from" [@@bs.val] (* es2015 *)
+external fromMap : 'a array_like -> ('a -> 'b [@bs]) -> 'b array = "Array.from" [@@bs.val] (* es2015 *)
+external unsafeFromMap : 'a -> ('b -> 'c [@bs]) -> 'c array = "Array.from" [@@bs.val] (* es2015 *)


This is also meaningless by reading its types

bobzhang · 2017-02-14T14:01:52Z

jscomp/others/js_array.ml

+
+external shift : 'a Js.undefined = "" [@@bs.send.pipe: 'a t as 'this]
+
+external sort : 'this = "" [@@bs.send.pipe: 'a t as 'this]


can you document that it is functional sort or imperative sort. so we have some names sortInPlace for imperative ones

bobzhang · 2017-02-14T14:03:04Z

jscomp/others/js_array.ml

 type 'a t = 'a array
+type 'a array_like


can you document where is array_like and array_iter used?

These were added primarily for the iterator functions, and if they're taken out there's currently no use for them. But there are many objects elsewhere that are array-like, arguments, NodeList and HTMLCollection to name a few. Without array_like and the Array.from function, these would all have to implement Array.from themselves as toArray. Unsure which is the right approach here.

bobzhang · 2017-02-14T14:04:04Z

jscomp/others/js_array.ml

+external copyWithinFrom : to_:int -> from:int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this] (* es2015 *)
+external copyWithinFromRange : to_:int -> start:int -> end_:int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this] (* es2015 *)
+
+external fill : 'a -> 'this = "" [@@bs.send.pipe: 'a t as 'this] (* es2015 *)


I am thinking of to make it more explicit like
Js.Array.Es6.copyWithin what do you think

I agree with making imperative functions explicit, but this specific case seems pretty explicit to me. I don't know what else it could mean. Unless we employ a more generic convention like fillMut, I don't know what I'd change this to.

bobzhang · 2017-02-14T14:05:23Z

jscomp/others/js_array.ml

-external length : 'a array -> int = ""
-[@@bs.get]
-
+external values : 'a array_iter = "" [@@bs.send.pipe: 'a t as 'this] (* es2015 *)


ah, so this is ES6 iterator, maybe we should not expose here, we need do some research to see how we could support iterator pattern in Buckle

bobzhang · 2017-02-14T22:24:46Z

How about fillInPlace -- in gneral, in OCaml, people would expect an imperative function to return `unit`, here JS version is not returning unit (for chainning?), I think having a clear hint that it is imperative will be useful reply@reply.github.com At: 02/14/17 17:22:15" data-digest="From: reply@reply.github.com At: 02/14/17 17:22:15" style=""> From: reply@reply.github.com At: 02/14/17 17:22:15 To: bucklescript@noreply.github.com Cc: HONGBO ZHANG (BLOOMBERG/ 731 LEX), comment@noreply.github.com Subject: Re: [bloomberg/bucklescript] Additions to js_string and js_array (#1203) @glennsl commented on this pull request. In jscomp/others/js_array.ml: > +external unsafeFrom : 'a -> 'b array = "Array.from" [@@bs.val] (* es2015 *) +external fromMap : 'a array_like -> ('a -> 'b [@bs]) -> 'b array = "Array.from" [@@bs.val] (* es2015 *) +external unsafeFromMap : 'a -> ('b -> 'c [@bs]) -> 'c array = "Array.from" [@@bs.val] (* es2015 *) +external isArray : 'a -> Js.boolean = "Array.isArray" [@@bs.val] (* es2015 *) +(* Array.of: seems pointless unless you can bind *) (* es2015 *) + +external length : 'a array -> int = "" [@@bs.get] + + +(* Mutator functions +*) +external copyWithin : to_:int -> 'this = "" [@@bs.send.pipe: 'a t as 'this] (* es2015 *) +external copyWithinFrom : to_:int -> from:int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this] (* es2015 *) +external copyWithinFromRange : to_:int -> start:int -> end_:int -> 'this = "copyWithin" [@@bs.send.pipe: 'a t as 'this] (* es2015 *) + +external fill : 'a -> 'this = "" [@@bs.send.pipe: 'a t as 'this] (* es2015 *) I agree with making imperative functions explicit, but this specific case seems pretty explicit to me. I don't know what else it could mean. Unless we employ a more generic convention like fillMut, I don't know what I'd change this to. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

glennsl · 2017-02-14T23:00:06Z

Right, I'll do that then. I also tried to do a rebase, which only seems to have made things worse. Guess I have to rebase -i and force push. Will experiment.

…2015

glennsl · 2017-02-15T01:41:26Z

Ok, rebase and force push seems to have gone well. And I've made come changes according to your comments.

bobzhang · 2017-02-15T14:36:01Z

looks good, thanks! I will enable v6 so you can test latest API.
Btw, how is merlin autocompletion works, will it show the documentation, if so, maybe we should add some docs there

glennsl · 2017-02-15T22:38:15Z

I've never seen merlin show doc comments. But I've also never seen doc comments in the wild, so I don't know which shape they ought to have.

253e94c v1.8.2 87111bf mark this 1.8.2.git 17ae02a Merge pull request #1323 from bradking/tolerate-phony-self-reference c0dde0f Restore tolerance of self-referencing phony build statements e679202 Factor ManifestParser options into a structure f69c785 v1.8.1 7738c19 mark this 1.8.1.git 6081e81 Merge pull request #1318 from atetubou/fix_for_maxpath_test 480f899 fix normalizer test for _MAX_PATH 0c67152 v1.8.0 b98941a mark this 1.8.0.git bfa7da5 Merge pull request #1313 from adzenith/patch-3 324a117 Add _Available since Ninja 1.4._ to `deps` and `recompact` 327d6ce Merge pull request #1314 from atetubou/fix_windows_path 86f606f Remove path component limit from input of CanonicalizePath in windows e6e4984 Add `deps` and `recompact` tools to manual 7bbc708 Merge pull request #1111 from bradking/detect-cycles-early 61e347e Merge pull request #1292 from gtristan/work-around-zero-mtime 721d2a2 Drop unnecessary cycle detection in Plan::AddTarget b6f020d Teach RecomputeDirty to detect cycles in the build graph a8b5cdc Add infrastructure for efficient walks through the `Edge` graph afe3beb Refactor RecomputeDirty to take a node instead of an edge 29a6e2f Simplify BuildTest.StatFailureAbortsBuild test case b34f744 Work around mtime being set to 0 sometimes 1029064 Merge pull request #1291 from colincross/fix1290 11a934d Fix segfault on edge with no inputs 00f69c1 Merge pull request #1156 from cdbennett/windows-binary-mode-output 0142023 Merge pull request #1287 from ivadasz/dragonfly_patches 89d9d62 Merge pull request #1281 from colincross/rebuild_on_error d806aa5 Add support for DragonFly. 04d886b Always rebuild on errors a127dda Add a test that fails if a rule updates it output file but fails 036003d Move stat metric to DiskInterface d6eb8ba Merge pull request #1280 from RedBeard0531/zsh_completion_explicit_file ad54532 Make zsh completion use explicitly specified ninja files 2fc5b9e Revert 78f893b f1551b3 Merge pull request #1271 from atetubou/faster_clparser f0bb90e Reduce GetFullPathName calls 75b3385 Fix for review 3b32002 Make clparser faster 08a3220 Add string_piece_util 586bb6d Merge pull request #1267 from atetubou/clparser_perftest 5a03f4e delete n 03d0a8b use us 12ca621 add gitignore 4eac806 remove util.h 292c89f Add clparser_perftest 0b0374e Merge pull request #1255 from tchajed/bind-localhost 8a32d21 browse: Bind to localhost by default fb3c700 Merge pull request #1231 from colincross/canon_perftest_fix 1df8b6e Merge pull request #1235 from refack/patch-1 f4ea9ca Merge pull request #1232 from nicolasdespres/fix-canon_perftest 4dc4f1d Merge pull request #1237 from danw/no_reload_with_restat abcd5f3 Support restat when rebuilding manifest d560c62 Need this to build on vs2017 d6ff22b Fix compilation error in canon_perftest. d611bd4 Make travis build everything 110dae2 Fix build in canon_perftest_fix 2993752 Merge pull request #1181 from DanielWeber/issue-1161 9e71431 Merge pull request #1226 from colincross/close-fds dd2b587 Close original pipe fd in subprocesses a36f96c Merge pull request #1225 from bippum/patch-1 29ea8ea fix broken link in hacking.md 95ddaa1 Merge pull request #1220 from phajdan/master 76abf78 Fix build with uclibc 7d705a3 Merge pull request #1213 from nico/copying 78f893b replace copyright placeholder, fixes #1212 3082aa6 Merge pull request #1206 from nico/versioninfo 32c82da windows: replace deprecated GetVersionEx with recommended replacement 2263f82 Merge pull request #1205 from nico/clangclformat 32c3625 fix a clang-cl -Wformat warning 256bf89 forgot to bump version in manual for 1.7.2 release a232a7f Merge pull request #1204 from nico/vernum 14f4794 fix RELEASING wrt manual.asciidoc process fdfff11 Merge pull request #1194 from ilor/depfile-empty-path 21fc120 Merge pull request #1182 from moosotc/master ef0616e Merge pull request #1203 from nico/reldoc 474e87d Update RELEASING -- manual.asciidoc no longer has a version number 04f4bc5 Merge pull request #1201 from nico/singlecommand 8b7155b teach -t commands to optionally print only the final command 46d9a95 Improve error message when a depfile contains a bad path 1cc730d Use uint64_t for slash_bits consistently 5153ced Use POSIX_SPAWN_USEVFORK if available c4b09e1 Allow more path components b042580 Write subprocess output to stdout in binary mode git-subtree-dir: vendor/ninja git-subtree-split: 253e94c

glennsl commented Feb 12, 2017

View reviewed changes

bobzhang reviewed Feb 13, 2017

View reviewed changes

bobzhang reviewed Feb 14, 2017

View reviewed changes

glennsl added 2 commits February 15, 2017 02:00

Added missing js string and js array functions up to and including ES…

27871f5

…2015

Review fixes

9b97611

bobzhang merged commit 6c12883 into rescript-lang:master Feb 15, 2017

glennsl deleted the lib branch February 16, 2017 02:09


		external shift : 'a Js.undefined = "" [@@bs.send.pipe: 'a t as 'this]

		external sort : 'this = "" [@@bs.send.pipe: 'a t as 'this]

Additions to js_string and js_array #1203

Additions to js_string and js_array #1203

Uh oh!

Conversation

glennsl commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Feb 12, 2017

Uh oh!

glennsl commented Feb 12, 2017

Uh oh!

glennsl commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Feb 13, 2017

Uh oh!

glennsl commented Feb 14, 2017

Uh oh!

bobzhang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Feb 14, 2017 via email

Uh oh!

glennsl commented Feb 14, 2017

Uh oh!

glennsl commented Feb 15, 2017

Uh oh!

bobzhang commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsl commented Feb 15, 2017

Uh oh!

Uh oh!

glennsl commented Feb 12, 2017 •

edited

Loading

glennsl commented Feb 12, 2017 •

edited

Loading

bobzhang commented Feb 15, 2017 •

edited

Loading