purescript-deprecated / purescript-maps

33 stars 33 forks source link

Add `unionsWith`? #55

Open hdgarrood opened 8 years ago

hdgarrood commented 8 years ago

This function doesn't currently exist, but I think it could, and would potentially be useful:

unionsWith :: forall f k v. (Foldable f, Ord k) => (v -> v -> v) -> f (Map k v) -> Map k v

It's probably very simple to define this using what's already exported, but I think it's still useful to have these sorts of things defined for you.

paf31 commented 8 years ago

:+1:

Cmdv commented 7 years ago

@paf31 just trying to pick up a ticket to get started with contributing, is this being worked on?

I assume this would need the unionsWith function added then exposed and tests?

paf31 commented 7 years ago

@Cmdv Yes, that's right, thanks!

Cmdv commented 7 years ago

-- edit: am I wrong in thinking that it is already exposed? here

@paf31 cool I'll give it a go 😄

hdgarrood commented 7 years ago

unionWith is slightly different, it only combines two maps. unionsWith could be used to take the union of an array with any number of maps inside it, for example.

Cmdv commented 7 years ago

@hdgarrood just got pointed out to me on IRC by garyb as well!! I'll probably get stuck when it comes to testing but I'll crack on :)

garyb commented 7 years ago

For tests, one property to check off the top of my head: the keys of all the maps that were unioned should exist in the resulting map.

garyb commented 7 years ago

Also just to note, we can drop Ord from the signature with the very latest -maps release (it's about 10 minutes old):

unionsWith :: forall f k v. Foldable f => (v -> v -> v) -> f (Map k v) -> Map k v
LiamGoodacre commented 7 years ago

@garyb how would that work??? 😕 With traverse it's fine as we're reconstructing a map in the same way we deconstruct it. But here we're having to combine maps - which means we need to insert the keys in the right place relative to each other.

garyb commented 7 years ago

Uh, yeah, I'm talking nonsense 😆... disregard that.