-
Notifications
You must be signed in to change notification settings - Fork 463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some utility functions to Belt Array #4734
Conversation
Signed-Off-By: Paul Tsnobiladzé <paul.tsnobiladze@gmail.com>
Added: mapReverse(WithIndex)(U) forEachReverse(withIndex)(U) keep(Map)Reverse(U) Signed-Off-By: Paul Tsnobiladzé <paul.tsnobiladze@gmail.com>
Signed-Off-By: Paul Tsnobiladzé <paul.tsnobiladze@gmail.com>
rescript implementation of Js Array.prototype.join() Signed-Off-By: Paul Tsnobiladzé <paul.tsnobiladze@gmail.com>
I am in general in favor of this except some comments. |
Ok thanks, there's no hurry anyway, I'll update my PR accordingly. |
It now works with arrays of any type, their elements being converted thanks to the toString function supplied as the final argument Signed-Off-By: Paul Tsnobiladzé <paul.tsnobiladze@gmail.com>
do you think it's a good idea to use an optional argument here, for example: map : 'a array -> ?reverse:bool -> ( 'a -> 'b) -> 'b array |
Good idea! that would indeed be quite elegant. I always forget you don't need an additional unit argument in such cases. That would help reduce the amount of functions needed in the API. We could also do the same for Maybe we could make a little survey on discourse? |
sounds good to me |
Well there seems to be no real consensus on this: The integration of these functions is up to you anyway, I'm OK with all these options, you're the Boss 😀 |
hi @tsnobip can we have the least controversial utility |
sure, would like me to revert the commit where I add the |
@tsnobip you can also branch from that commit and make a separate PR, either works for me, thanks |
ok I created a separate PR (#4748) to make things easier and cleaner. |
Could be revisited now if rebased on master, assuming there's still interest. |
Yeah I think those functions can still be interesting from time to time. I'll rebase it on master when I have some time. |
I added the reverse version of
map
,forEach
andkeep
.I fixed some typos and examples of the docs.
I also added a rescript implementation of
joinWith
(it's around 3 times faster thanJs.Array.joinWith
on common examples, and still faster for arrays up to around 10,000 elements (result string of around 100,000 characters), not sure this function should be used for bigger arrays anyway. This function is only applicable forstring array
, not sure if it should be placed insideBelt.Array
since the other functions apply to polymorphic arrays.I couldn't find any test files for Belt so I just tested the functions manually. I'll add some tests if needed.