Add 'intersectionWith', 'mergeWith' combinators

[mitchellwrosen]

In case you want it, this patch adds intersectionWith and mergeWith (#158) (though I prefer the names intersection and merge) Sorry about all the whitespace changes, my editor deletes trailing spaces on save.

[HeinrichApfelmus]

Hm, I'm not sold on the intersectionWith combinator. If anything, I would prefer mergeWith and both unionWith and coincidence defined in terms of it.

[mitchellwrosen]

I've updated this PR to remove intersectionWith and re-define unionWith in terms of mergeWith

[mitchellwrosen]

Any thoughts on this @HeinrichApfelmus?

[HeinrichApfelmus]

Looks good! Can you add an entry to the changelog and your name to the contributors list (if not already there)? I'll merge if you notify me after that.

[mitchellwrosen]

Sure! Can I make one last appeal to shorten the names to union and merge?

My reasoning is:

• Neither name is imported by Prelude.

• In unionWith, it's obvious (to me) what the given function does. I'd be similarly unhappy if filter was instead called filterWith. Of course it filters with a predicate because it's an argument to the function.

  I know that a previous version of reactive-banana exported both union and unionWith, but since union is gone, we should reclaim its nice, short name :)

• For mergeWith, the "with" is awkwardly referring to three separate arguments.

To avoid a major version bump (if that's a concern) I'd also be totally happy with leaving unionWith around, but deprecated in favor of union.

Or, if you prefer the names unionWith and mergeWith, that's fine too :)

[HeinrichApfelmus]

The main reasoning behind the names was that they be consistent with the names in the Data.Map module. But there may be better arguments in favor of the new names.

Anyway, since name changes require a major version bump and many changes in dependent code, I would like to collect them in a single major release, possibly 2.0. Could you add the arguments to #147 ?

[mitchellwrosen]

Sure, I'll move the discussion. There's also a new map-merge module that I just discovered:

https://hackage.haskell.org/package/containers-0.5.11.0/docs/Data-Map-Merge-Lazy.html

It provides a merge function:

merge    
  :: Ord k	 
  => SimpleWhenMissing k a c	
  -> SimpleWhenMissing k b c	
  -> SimpleWhenMatched k a b c	
  -> Map k a	
  -> Map k b	
  -> Map k c	 

So, name change issue aside, one notable difference is the argument order.

Perhaps mergeWith here should be changed to reflect this:

mergeWith :: (a -> Maybe c) -> (b -> Maybe c) -> (a -> b -> Maybe c) -> Event a -> Event b -> Event c

[HeinrichApfelmus]

I like the new order of arguments better.

[mitchellwrosen]

Forgot to mention here, I force-pushed with the updated arg order.

[mitchellwrosen]

I'm still interested in getting this merged, is there anything else I can do?

[ocharles]

If you can add a changelog entry I'm happy to merge this, as it seems to have reached consensus.

[mitchellwrosen]

@ocharles Sure thing, done

[mitchellwrosen]

I'm going to merge this, there is just that one outstanding rename suggestion I made in the other thread (nix the *With suffixes).