-
Notifications
You must be signed in to change notification settings - Fork 11
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.Extra.splice #22
Comments
I like the flexibility of b a lot. Because this helper will probably be used rarely, I'd even prefer making it more clear with something like Array.fromList [ 1, 2, 3, 4, 5 ]
|> updateSlice { start = 2, length = 2 } Array.reverse
--> Array.fromList [ 1, 2, 4, 3, 5 ] which still doesn't seem overly verbose in my eyes |
Also in your first example, are you sure it should be Array.fromList [ 1, 2, 3, 4, 5 ]
|> splice 2 0 (Array.singleton 33)
--> Array.fromList [ 1, 2, 33, 4, 5 ] and not Array.fromList [ 1, 2, 3, 4, 5 ]
|> splice 2 0 (Array.singleton 33)
--> Array.fromList [ 1, 2, 33, 3, 4, 5 ] |
Can you clarify: would you just like to have
My bike shed for this was We don’t have records like this in the API, but perhaps that’s a shame. Maybe we could even have multiple functions that take/return an
👍 that’s why doctests are a thing. |
Good question. I was originally thinking about having only b. The only consideration would be that the splice that doesn't take the existing, replaced section is faster since elm isn't a lazy language. We could of course have an ugly API like : { start : Int, length : Int } -> ((() -> Array a) -> Array a) -> Array a -> Array a
Array.fromList [ 1, 2, 3, 4, 5 ]
|> sectionUpdate { start = 2, length = 2 }
(\section -> Array.reverse (section ()))
--> Array.fromList [ 1, 2, 4, 3, 5 ] But it's simpler to have one helper for update and one for replace as you described.
I don't hate "splice" as a name for the replace version of this operation; not sure it fits the update one.
I agree with your suggestion of putting splice/section/sub/... first :) Another possible API could be updateBefore : Int -> (Array a -> Array a) -> Array a -> Array a
updateFrom : Int -> (Array a -> Array a) -> Array a -> Array a
Array.fromList [ 1, 2, 3, 4, 5 ]
|> updateFrom 2 (updateBefore 2 Array.reverse))
--> Array.fromList [ 1, 2, 4, 3, 5 ] which is even more versatile/atomic/flexible and possibly more intuitive (since array extra already has slice from a start index and before and end index. splitAt also works quite similar) |
splice : Int -> Int -> Array a -> Array a -> Array a
The OG ultimate array manipulation function. Start at an index, remove a number of items and insert items in their place.
Motivating use case
Alternative designs
a) returns the deleted items. This is rarely useful (as you can easily get the items to be deleted with a slice call) and suffers from ambiguity on which are the deleted items and which is the returned array.
b) allows the inserted items to depend on the deleted items. If you think of splice as a sort of generalised CRUD function over arrays, then this version gives you the Update. It also composes nicely with Array.map to implement a map on only a part of an array. I personally quite like this version. However, for a lot of the normal use cases it introduces a fair amount of ceremony. Also getting the deleted range is again trivial with a slice, so not particularly necessary to build it like this.
c) is a lot nicer for literal use cases. I don’t really have a way to evaluate this, but I think the type mixing is aesthetically not particularly nice. But if there was a strong argument for this, I would be willing to consider it.
d) this base signature suffers from having pairs of arguments with different meanings where the programmer just needs to know the order. This version disambiguates the call sites. I think this is somewhat mitigated in that this is a well known kind of function and the argument order tends to be like this. Also we don’t have much precedent in -extra for this style of API.
Rationale
If this didn’t exist, how would one implement this using existing functionality?
or some such. I’m not particularly confident in the correctness of this implementation and neither do I think that its intent is particularly obvious. I would also be wondering about performance and whether this is indeed a decent implementation. As such I think having it in a well tested and well performing library would be preferable.
The text was updated successfully, but these errors were encountered: