Argument precedence of Record and Builder merging

[matthew-hilty]

I've noticed that the merge-like functions in 'Record' favor their first argument, whereas those in 'Record.Builder' favor their second. I'm guessing that a user would at least initially assume that Record merging and Builder merging would have identical semantics except for the function/Builder difference. Could this semantic distinction and its rationale be documented? Alternatively, could either Record merging or Builder merging be updated so that the two function sets are consistent with each other? (As far as I can tell, there's no pressing problem motivating this change, but consistency is often good for its own sake, and it might help to improve grokkability and to prevent merger-precedence bugs.)

-- Record
merge
  :: forall r1 r2 r3 r4
   . Union r1 r2 r3
  => Nub r3 r4
  => Record r1
  -> Record r2
  -> Record r4
merge l r = runFn2 unsafeUnionFn l r

-- Record.Builder
merge
  :: forall r1 r2 r3 r4
   . Row.Union r1 r2 r3
  => Row.Nub r3 r4
  => Record r2
  -> Builder (Record r1) (Record r4)
merge r2 = Builder \r1 -> runFn2 unsafeUnionFn r1 r2

[hdgarrood]

I think the makes sense as it is; the fact that a Builder is actually a function is kind of an implementation detail, so to me, the merge function from Record.Builder is really function of just one argument which returns a Builder. From this perspective I think they are consistent.

Some more detailed docs for the Record.Builder module would definitely be good though; if the docs explicitly mentioned what happens in the case where both of the records involved in a merge have a particular property, that would be good, and some examples would be good too.

[matthew-hilty]

It's kind of a trifling matter, I recognize. Except for some documentation, probably not worth much attention.

I guess I just assumed, before I looked at the type signatures more closely, that x0 and x1 below would be equivalent, that swapping out R.merge for B.merge and F.apply for B.build would improve efficiency but otherwise leave everything else unchanged.

I admit, I'm still confused about why Record.merge and Builder.merge are designed differently -- their type signatures and use cases seem too similar to me to be mere coincidence. It's just an implementation detail, and it can be worked around, but it's thrown me for a loop anyway.

import Prelude
import Data.Function as F
import Record as R
import Record.Builder as B

x0 :: { a0 :: Int, a1 :: Int, a2 :: Int }
x0 = F.apply (R.merge { a0: 2, a2: 2 } <<< R.merge { a0: 1, a1: 1 }) { a0: 0 }

x1 :: { a0 :: Int, a1 :: Int, a2 :: Int }
x1 = B.build (B.merge { a0: 2, a2: 2 } <<< B.merge { a0: 1, a1: 1 }) { a0: 0 }

-- x0 == { a0: 2, a1: 1, a2: 2 }
-- x1 == { a0: 0, a1: 1, a2: 2 }

x2 :: { a0 :: Int, a1 :: Int, a2 :: Int }
x2 = F.apply (flip R.merge { a0: 2, a2: 2 } <<< flip R.merge { a0: 1, a1: 1 }) { a0: 0 }

-- x2 == { a0: 0, a1: 1, a2: 2 }

-- (Because the `Builder` category doesn't support products, there doesn't seem to
-- be an analogous way to flip the arguments in the `x1` case to make `B.merge` behave
-- like `R.merge` -- at least nothing obvious comes to mind.)

(Thanks for the feedback by the way.)

[hdgarrood]

Oh sorry - I've just thought this through again and on reflection I think the current behaviour of Record.Builder.merge is a bit surprising. I think the current one is probably more of a "set these properties to these default values if they don't already exist in the record being built" function, so perhaps it ought to be called withDefaults or something, and the name merge should be reserved for a builder which does override values which already exist in the record being built. Thoughts @paf31?

[garyb]

As a spectator, that sounds sensible to me.

[kl0tl]

Is this something we should do in 0.14? I agree that it is indeed confusing for build (Record.Builder.merge r1) r2 to behave differently than Record.merge r1 r2, on the other hand some programs might depend on the current behaviour.

[JordanMartinez]

I think so