Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

tsnobip
Copy link
Contributor

@tsnobip tsnobip commented Sep 29, 2020

I added the reverse version of map, forEach and keep.

I fixed some typos and examples of the docs.

I also added a rescript implementation of joinWith (it's around 3 times faster than Js.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 for string array, not sure if it should be placed inside Belt.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.

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>
@bobzhang
Copy link
Member

I am in general in favor of this except some comments.
Note I am going to take an vacation, so I will merge after it. Other reviews are welcome

@tsnobip
Copy link
Contributor Author

tsnobip commented Sep 30, 2020

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>
@tsnobip tsnobip marked this pull request as draft September 30, 2020 10:45
@bobzhang
Copy link
Member

bobzhang commented Oct 8, 2020

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

@tsnobip
Copy link
Contributor Author

tsnobip commented Oct 8, 2020

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 reduceReverse for coherence.

Maybe we could make a little survey on discourse?

@bobzhang
Copy link
Member

bobzhang commented Oct 8, 2020

Maybe we could make a little survey on discourse?

sounds good to me

@tsnobip
Copy link
Contributor Author

tsnobip commented Oct 12, 2020

Well there seems to be no real consensus on this:
https://forum.rescript-lang.org/t/api-convention-for-reverse-versions-of-belt-functions/457/
Among 22 respondents, the bigger part would prefer to have extra functions, a smaller part would like to have a flag in the existing functions, some suggest to put those functions inside a Reverse module (the same could be done with the WithIndex functions but that would be a breaking change I guess) and some would even prefer not to have these functions at all in Belt.

The integration of these functions is up to you anyway, I'm OK with all these options, you're the Boss 😀

@bobzhang
Copy link
Member

hi @tsnobip can we have the least controversial utility toString merged first?

@tsnobip
Copy link
Contributor Author

tsnobip commented Oct 13, 2020

sure, would like me to revert the commit where I add the reverse versions and rebase to trunk?

@bobzhang
Copy link
Member

@tsnobip you can also branch from that commit and make a separate PR, either works for me, thanks

@tsnobip
Copy link
Contributor Author

tsnobip commented Oct 14, 2020

ok I created a separate PR (#4748) to make things easier and cleaner.

@cristianoc
Copy link
Collaborator

Could be revisited now if rebased on master, assuming there's still interest.

@tsnobip
Copy link
Contributor Author

tsnobip commented Jul 1, 2022

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.

@cristianoc cristianoc closed this Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants