Skip to content

Override existing fields with Record.Builder.merge and Record.Builder.union #73

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.

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

Conversation

kl0tl
Copy link
Member

@kl0tl kl0tl commented Dec 18, 2020

This pull request updates Record.Builder.merge and Record.Builder.union so that they behave more like Record.merge and Record.union: fields from the argument override those of the record being built in case of overlaps.

I also added a Record.Builder.flip function that can be used with both Record.Builder.merge and Record.Builder.union instead of renaming the existing Record.Builder.merge function to withDefaults and the existing Record.Builder.union to something else.

Closes #55.

assert' "Record.Builder.union" $
let { x, y, z } = Builder.build (Builder.union { x: 1, y: "y" }) { y: 2, z: true }
:: { x :: Int, y :: String, y :: Int, z :: Boolean }
in x == 1 && y == "y" && z
Copy link
Contributor

Choose a reason for hiding this comment

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

Could there be a second test here that verifies that this test works? Likewise for the flip version

in x ==1 && y == 2 && z

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I’m not sure to understand what you mean: y == 2 doesn’t hold here (nor does y == "y" for the other test).

Copy link
Contributor

@JordanMartinez JordanMartinez Dec 23, 2020

Choose a reason for hiding this comment

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

Let me clarify. This function doesn't remove duplicate labels.

let 
  { x, y, z } = Builder.build (Builder.union { x: 1, y: "y" }) { y: 2, z: true }
    :: { x :: Int
       , y :: String -- first 'y'
       , y :: Int    -- second 'y'
       , z :: Boolean 
       }

So, when you write let { x, y, z } = Record.Builder.merge ...., the y's type could be String or Int. But due to the next part in the test...

in x == 1 && y == 2 && z

... it's my understanding that the compiler will infer that the y in { x, y, z } refers to the (y :: Int) row, not the (y :: String) row.

So, I was asking whether a duplicate of this test should be added, where you replace the y == "y" part with y == 2.

However, looking at this again, I think my understanding is off. While the returned record does have a (y :: String) row, the runtime representation of the underlying value doesn't actually have that. union differs from merge in that it doesn't require a Nub constraint as one might wish to defer this constraint until a later time period.

If you did duplicate the test and used y == "y", I'm assuming that would result in a runtime error, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked this out locally and added that test just to see what would happen. It just produces a compiler error:

[1/1 TypesDoNotUnify] test/Main.purs:80:23

  80      in x == 1 && y == 2 && z
                            ^
  
  Could not match type
  
    Int
  
  with type
  
    String
  
  while checking that type Int
    is at least as general as type String
  while checking that expression 2
    has type String
  in value declaration main

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll wait to hear your feedback on the test though.

@JordanMartinez JordanMartinez merged commit 4e864ba into purescript:master Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argument precedence of Record and Builder merging
3 participants