Skip to content

[#29] Add fromFoldableWithIndex #30

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 24, 2022
Merged

[#29] Add fromFoldableWithIndex #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e6ade6
Add `fromFoldableWithIndex`
pete-murphy Oct 19, 2022
c17f1cc
Add tests for `fromFoldableWithIndex`
pete-murphy Oct 19, 2022
76c0790
Fix test
pete-murphy Oct 20, 2022
995093b
Document behavior on key collision
pete-murphy Oct 20, 2022
eacb14c
Change API for fromFoldableWithIndex functions
pete-murphy Oct 21, 2022
56e9241
More general `fromFoldableWithIndexWith`
pete-murphy Oct 21, 2022
489634e
Remove fromFoldableWithIndexWith
pete-murphy Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Notable changes to this project are documented in this file. The format is based
Breaking changes:

New features:
- Add `fromFoldableWithIndex` (#30 by @ptrfrncsmrph)

Bugfixes:

Expand Down
1 change: 1 addition & 0 deletions bower.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"devDependencies": {
"purescript-assert": "^6.0.0",
"purescript-minibench": "^4.0.0",
"purescript-ordered-collections": "^3.0.0",
"purescript-quickcheck": "^8.0.1"
}
}
10 changes: 9 additions & 1 deletion src/Foreign/Object.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module Foreign.Object
, toAscUnfoldable
, fromFoldable
, fromFoldableWith
, fromFoldableWithIndex
, fromHomogeneous
, delete
, pop
Expand Down Expand Up @@ -49,7 +50,7 @@ import Control.Monad.ST as ST
import Data.Array as A
import Data.Eq (class Eq1)
import Data.Foldable (class Foldable, foldl, foldr, for_)
import Data.FoldableWithIndex (class FoldableWithIndex)
import Data.FoldableWithIndex (class FoldableWithIndex, forWithIndex_)
import Data.Function.Uncurried (Fn2, runFn2, Fn4, runFn4)
import Data.FunctorWithIndex (class FunctorWithIndex)
import Data.Maybe (Maybe(..), maybe, fromMaybe)
Expand Down Expand Up @@ -221,6 +222,13 @@ 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 a `String`-indexed foldable collection
fromFoldableWithIndex :: forall f a. FoldableWithIndex String f => f a -> Object a
fromFoldableWithIndex l = runST do
s <- OST.new
forWithIndex_ l \k v -> OST.poke k v s
pure s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on adding a variant fromFoldableWithIndex' that assumes the k is already a String?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a

I 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 v

basically support the (probably) common case with fromFoldableWithIndex and then give more granular control with fromFoldableWithIndexWith (not sure about that naming 😄 )

Footnotes

  1. https://discord.com/channels/864614189094928394/865617619464749081/887326007105814548
    https://discord.com/channels/864614189094928394/867149806178664478/1032345440248668242 ↩

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pete-murphy pete-murphy Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose

fromFoldableWithIndexWith
  :: forall f k a b
   . FoldableWithIndex k f
  => (k -> a -> Tuple String b)
  -> (b -> b -> b)
  -> f a
  -> Object b

would be the most general variation, but I don't know if that's too complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a

make sense?

foreign import _lookupST :: forall a r z. Fn4 z (a -> z) String (STObject r a) (ST r z)

-- | Create an `Object a` from a foldable collection of key/value pairs, using the
Expand Down
18 changes: 18 additions & 0 deletions test/Test/Foreign/Object.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Data.FoldableWithIndex (foldlWithIndex, foldrWithIndex, foldMapWithIndex)
import Data.Function (on)
import Data.List as L
import Data.List.NonEmpty as NEL
import Data.Map as M
import Data.Maybe (Maybe(..), fromMaybe)
import Data.Semigroup.First (First(..))
import Data.Semigroup.Last (Last(..))
Expand Down Expand Up @@ -150,6 +151,14 @@ objectTests = do
quickCheck (O.lookup "1" nums == Just "one" <?> "invalid lookup - 1")
quickCheck (O.lookup "2" nums == Nothing <?> "invalid lookup - 2")

log "fromFoldableWithIndex"
do
let numsMap = M.fromFoldable [Tuple "0" "zero", Tuple "1" "one", Tuple "2" "two"]
nums = O.fromFoldableWithIndex 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 == Just "two" <?> "invalid lookup - 2")

log "fromFoldableWith const [] = empty"
quickCheck (O.fromFoldableWith const [] == (O.empty :: O.Object Unit)
<?> "was not empty")
Expand All @@ -170,6 +179,15 @@ objectTests = do
let f m1 = O.fromFoldable ((O.toUnfoldable m1) :: L.List (Tuple String Int)) in
O.toUnfoldable (f m) == (O.toUnfoldable m :: L.List (Tuple String Int)) <?> show m

log "fromFoldableWithIndex = id"
quickCheck $ \(TestObject m :: _ Int) ->
O.fromFoldableWithIndex m == identity m <?> show m

log "fromFoldableWithIndex = fromFoldable . toUnfoldable"
quickCheck $ \(TestObject m) ->
let f m1 = O.fromFoldable ((O.toUnfoldable m1) :: L.List (Tuple String Int)) in
O.fromFoldableWithIndex m == f m <?> show m

log "fromFoldableWith const = fromFoldable"
quickCheck $ \arr -> O.fromFoldableWith const arr ==
O.fromFoldable (arr :: L.List (Tuple String Int)) <?> show arr
Expand Down