-
Notifications
You must be signed in to change notification settings - Fork 471
Add Array.removeInPlace #7321
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 Array.removeInPlace #7321
Changes from 3 commits
1d2fa81
7d8f261
5735137
8fb2a5b
245a57f
81db867
9847784
3e501a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,3 +109,36 @@ Test.run( | |
|
|
||
| Test.run(__POS_OF__("last - with items"), [1, 2, 3]->Array.last, eq, Some(3)) | ||
| Test.run(__POS_OF__("last - empty"), []->Array.last, eq, None) | ||
|
|
||
| { | ||
| let array = [] | ||
| array->Array.splice(~start=1, ~remove=0, ~insert=["foo"]) | ||
| Test.run(__POS_OF__("splice - Insert no delete"), array, eq, ["foo"]) | ||
| } | ||
|
|
||
| { | ||
| let array = ["bar", "baz"] | ||
| Test.run( | ||
| __POS_OF__("splice - Insert and delete"), | ||
| (array->Array.splice(~start=1, ~remove=1, ~insert=["foo"]), array), | ||
| eq, | ||
| ( | ||
| // Even though original .splice returns an array with the removed items, | ||
| // the binding returns unit so there's no confusion about it mutating the original array. | ||
| (), | ||
| ["bar", "foo"], | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| { | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.