Add short circuiting functions of elem, notElem, find, findMap, scanl, and scanr
#189
Conversation
|
Can I get a review on this? |
| assert $ (A.find (_ /= 1) [1, 2, 1]) == Just 2 | ||
| assert $ (A.find (_ == 3) [1, 2, 1]) == Nothing | ||
|
|
||
| log "findMap should return the mapping of the first element that satisfies the given predicate" |
There was a problem hiding this comment.
I think it would be good to also add a test case for an array where there is more than one element which satisfies the predicate here.
There was a problem hiding this comment.
Fixed in latest commits
There was a problem hiding this comment.
Sorry, I should have been clearer: I think we should have a test which rules out the possibility that our findMap actually returns the last match.
There was a problem hiding this comment.
I've updated the tests to account for that.
| [false, true, false, true] | ||
|
|
||
| log "scanl should return an array that stores the accumulated value at each step" | ||
| assert $ A.scanl (+) 0 [1,2,3] == [1, 3, 6] |
There was a problem hiding this comment.
Can we test that these agree with the Foldable versions please? It's probably not necessary to compare with the Foldable versions for the other functions, but this one differs from Haskell and so there's more than one "sensible" option for how to implement it, so I think it's worth being a little bit more careful/deliberate in the tests.
There was a problem hiding this comment.
Could you clarify what you mean by this? The tests I used were taken from the scanl docs and scanr docs
There was a problem hiding this comment.
Rereading this again, I think you mean this:
assert $ A.scanl (+) 0 [1,2,3] == scanl (+) 0 [1,2,3]
src/Data/Array.purs
Outdated
| scanl :: forall a b. (b -> a -> b) -> b -> Array a -> Array b | ||
| scanl = scanlImpl | ||
|
|
||
| foreign import scanlImpl :: forall a b. (b -> a -> b) -> b -> Array a -> Array b |
There was a problem hiding this comment.
Since the FFI implementation is ready to use as-is, we can just import it as scanl straight away; I don't think there's any need to alias it.
There was a problem hiding this comment.
Fixed in latest commits
|
I believe I've addressed all your comments. |
3 participants
See #173 (comment)