Add the Array module #27
Conversation
|
This is great thanks! What's happening with the odoc stuff? That's suddenly very unreadable. We're also at a place where there's like 4 of us using different ocamlformat settings, and this PR is suffering from it in particular. Could you take a glance at #7? |
|
Sure, I will look at getting ocamlformat set up. What do you mean by odoc stuff? |
This stuff here: https://github.com/darklang/tablecloth/pull/27/files#diff-484204121b3d8dfb8718cebe802a55a9R2 |
…y for some nice examples / tests
Dean177
force-pushed
the
add-array-module
branch
from
beb13dd
to
9f21a7e
Compare
|
I have rebased out the changes to |
|
Great, thanks. So a thing that I just noticed is that this array is mutable. The Elm one is not of course. The Belt one is mutable, as are the Batteries one and the OCaml one. I'd love to hear some thoughts on this. |
|
hey Dean, good job so far. As you can see in the benchmark in PR #26, using flip to force functions from Ocaml that use accumulator first to conform to the Elm Style of element first is a bad idea™ |
| Base.Array.fold ~f:(fun b a -> f a b) ~init:initial a | ||
|
|
||
| let fold_left = foldLeft | ||
|
|
There was a problem hiding this comment.
| let foldl = foldLeft | |
| Base.Array.fold_right ~f ~init:initial a | ||
|
|
||
| let fold_right = foldRight | ||
|
|
There was a problem hiding this comment.
Its a slightly different signature (initial rather than init).
There was a problem hiding this comment.
| let foldr = foldRight | |
There was a problem hiding this comment.
Oh you mean add the foldr alias in addition to foldRight and fold_right.
This was an intentional deviation from Elms standard library as I felt it was more in keeping with Elms guidelines on API's: https://package.elm-lang.org/help/design-guidelines#use-human-readable-names
In this particular case the Elm docs even go on to explain that foldr means "fold from the right".
There was a problem hiding this comment.
im in a rush, gonna read it when i have time. My thought was we have indexedMap/mapi
There was a problem hiding this comment.
I think we should remove mapi and all synonyms - have just one way to do it.
|
I think we should stick with Array being mutable and potentially offer an immutable alternative with a name other than Array. I figure we need to inter-operate on two fronts: Javascript and native OCaml, both of those have mutable Arrays and having a collection with the same name name but different behaviour would be potentially confusing. |
|
OK, happy to go with the mutable version, thanks for the opinion. If you want to go through @j-m-hoffmann's review/suggestions and either fix them or decide not to, then we can get this merged. |
Dean177
force-pushed
the
add-array-module
branch
from
38a10d6
to
af6c6a4
Compare
…place of `Array.of_list` for `bs`.Remove the now unused `Caml` module.Use Base.Array.init in place of Array.init for native
Dean177
force-pushed
the
add-array-module
branch
from
af6c6a4
to
9381b48
Compare
|
@j-m-hoffmann are you happy your comments have been addressed? If so I think that means this is ready @pbiggar |
|
Gonna merge this now, to avoid future conflicts. @j-m-hoffmann, if you wanna comment on this PR we can get the addressed after. |
Add the Array module
I started from @j-m-hoffmann 's pull request #14 and in an effort to have a more manageable PR I
ListorBase/BeltI also added a teeny Int module since it made writing the examples / tests easier.
This differs from Elms Array module in the following ways:
singleton,range,map2,flatMap,setany,all,append,concatenate,flatten,intersperse,reverse,reverseInPlace&forEachfunctions.pushindexedMap->mapWithIndexfoldl->foldLeft&foldr->foldRight, additionally both functions have slightly different signaturessliceuses a named and optional parameter