[#29] Add fromFoldableWithIndex
#30
Conversation
pete-murphy
force-pushed
the
pm/29/add-from-foldable-with-index
branch
from
25fa7a3 to
c17f1cc
Compare
test/Test/Foreign/Object.purs
Outdated
| log "fromFoldableWithIndex & key collision" | ||
| do | ||
| let numsMap = M.fromFoldable [Tuple 0 "zero", Tuple 1 "what", Tuple 1 "one"] | ||
| nums = O.fromFoldableWithIndex show numsMap | ||
| quickCheck (O.lookup "0" nums == Just "zero" <?> "invalid lookup - 0") | ||
| quickCheck (O.lookup "1" nums == Just "one" <?> "invalid lookup - 1") | ||
| quickCheck (O.lookup "2" nums == Nothing <?> "invalid lookup - 2") |
There was a problem hiding this comment.
Oops, this isn't actually testing key collision on the object, since the M.fromFoldable handles that. I'll update.
src/Foreign/Object.purs
Outdated
| @@ -221,6 +222,14 @@ fromFoldable l = runST do | |||
| ST.foreach (A.fromFoldable l) \(Tuple k v) -> void $ OST.poke k v s | |||
| pure s | |||
|
|
|||
| -- | Create an `Object a` from an indexed foldable collection, using the | |||
| -- | specified function for converting the index to `String` | |||
There was a problem hiding this comment.
This should also state something about key collisions using the later values.
| s <- OST.new | ||
| forWithIndex_ l \k v -> OST.poke (f k) v s | ||
| pure s | ||
|
|
There was a problem hiding this comment.
Thoughts on adding a variant fromFoldableWithIndex' that assumes the k is already a String?
There was a problem hiding this comment.
I'm now thinking that makes sense as the default API, based on the two use cases1 cited in the issue. They both say that Object.fromFoldableWithIndex would be useful specifically in the case of FoldableWithIndex k f ~ Map String.
fromFoldableWithIndex :: forall f a. FoldableWithIndex String f => f a -> Object aI think non-String keys would probably be the less common case, and in that case you might as well also throw in a function for combining values on conflict (instead of defaulting to overriding earlier values, which could be undesirable)
fromFoldableWithIndexWith :: forall f k v. FoldableWithIndex k f => (k -> String) -> (v -> v -> v) -> f v -> Object vbasically support the (probably) common case with fromFoldableWithIndex and then give more granular control with fromFoldableWithIndexWith (not sure about that naming 😄 )
Footnotes
There was a problem hiding this comment.
I think we can skip fromFoldableWithIndexWith because you could mapKeys (or equivalent) the input first.
It would be more efficient to convert while doing this, but you could say the same about accepting mapping functions in fromFoldables in general if we wanted to start going down that route.
There was a problem hiding this comment.
I suppose
fromFoldableWithIndexWith
:: forall f k a b
. FoldableWithIndex k f
=> (k -> a -> Tuple String b)
-> (b -> b -> b)
-> f a
-> Object bwould be the most general variation, but I don't know if that's too complicated.
There was a problem hiding this comment.
It would be more efficient to convert while doing this, but you could say the same about accepting mapping functions in
fromFoldables in general if we wanted to start going down that route.
Didn't see your comment til after I posted mine, that makes sense to skip fromFoldableWithIndexWith. So would just adding
fromFoldableWithIndex :: forall f a. FoldableWithIndex String f => f a -> Object amake sense?
pete-murphy
force-pushed
the
pm/29/add-from-foldable-with-index
branch
from
7d5b8b6 to
489634e
Compare
Description of the change
Add
fromFoldableWithIndexand tests. Fixes #29.Checklist: