Add Array.removeInPlace #7321
Add Array.removeInPlace #7321
Conversation
runtime/Stdlib_Array.res
Outdated
| @@ -103,11 +103,14 @@ external copyWithin: (array<'a>, ~target: int, ~start: int, ~end: int) => array< | |||
| @send external shift: array<'a> => option<'a> = "shift" | |||
|
|
|||
| @variadic @send | |||
| external splice: (array<'a>, ~start: int, ~remove: int, ~insert: array<'a>) => unit = "splice" | |||
| external splice: (array<'a>, ~start: int, ~remove: int, ~insert: array<'a>) => array<'a> = "splice" | |||
There was a problem hiding this comment.
I think we willingly ignore the results of functions that mutate their arguments to make it clear that they have side effects, I'd likely rename it to something more explicit otherwise
| external splice: (array<'a>, ~start: int, ~remove: int, ~insert: array<'a>) => array<'a> = "splice" | |
| external spliceInPlace: (array<'a>, ~start: int, ~remove: int, ~insert: array<'a>) => array<'a> = "splice" |
There was a problem hiding this comment.
I agree. What others think?
There was a problem hiding this comment.
Yes, non breaking and the current bindings do have a purpose. So I agree with this comment.
There was a problem hiding this comment.
Do you mean to keep the current splice as it is and add a new bindings with the return type?
There was a problem hiding this comment.
yeah I'd add a new external with the return type.
There was a problem hiding this comment.
Is it OK to have the version in rescript breaks some backward compatibility? If so I think making Array.splice behave as JS does would provide a smoother experience. Unlike Array.sort the current binding of Array.splice is more restricting and less powerful. For Array.sort one can often fall back on toSorted but splice is different.
There was a problem hiding this comment.
I removed the change from the PR. It's ready for review now.
DZakh
changed the title
runtime/Stdlib_Array.resi
Outdated
|
|
||
| @send | ||
| external removeInPlace: (array<'a>, int, @as(1) _) => unit = "splice" | ||
|
|
There was a problem hiding this comment.
nitpicking, but wouldn't it be better to move the tests here as docstring tests? At least they would help documenting the function.
Whatever we decide to do, I'd add some documentation here anyway, at least to document that the second argument is an index. Maybe it's a sign we should use a labelled argument here (~start, ~index?) like the other functions with indices here, what do you guys think? @zth ?
There was a problem hiding this comment.
Added a documentation comment 👍
Regarding labeled arguments I think it's consistent for the array API to not use it when we have a single argument which is an index. But it would definitely make sense if we had more arguments like:
@send
external removeMultipleInPlace: (array<'a>, ~start: int, ~number: int) => unit = "splice"
There was a problem hiding this comment.
many functions with just one int parameter in the Stdlib.Array module are labeled, it might be worth making all of this more consistant, but this could definitely be in another PR.
|
I fixed the comments, but tests are failing for some weird error. Could somebody help me with them? |
are you sure you ran |
| let array = [] | ||
| array->Array.removeInPlace(0) | ||
| Test.run(__POS_OF__("removeInPlace - empty"), array, eq, []) | ||
| } | ||
|
|
||
| { | ||
| let array = ["Hello", "Hi", "Good bye"] | ||
| array->Array.removeInPlace(1) | ||
| Test.run(__POS_OF__("removeInPlace - from middle"), array, eq, ["Hello", "Good bye"]) | ||
| } |
There was a problem hiding this comment.
should we keep these tests here given they're already in the docstring tests?
There was a problem hiding this comment.
Wow, are they automated?
There was a problem hiding this comment.
@DZakh yes, that's the whole point of those docstring tests :)
Co-authored-by: Paul Tsnobiladzé <[email protected]>
|
@tsnobip Done 👍 |
* Add Array.removeInPlace * Update changelog * Add documentation * Commit new tests output * Fix analysis test * Update tests/analysis_tests/tests/test.sh Co-authored-by: Paul Tsnobiladzé <[email protected]> * Remove duplicated tests --------- Co-authored-by: Paul Tsnobiladzé <[email protected]>
Splice returns an array of removed elements. I don't remember where I saw an issue, but I think we decided to change the binding to return an array.