Skip to content
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

Add record embedding #1

Open
fonghou opened this issue Mar 19, 2021 · 6 comments
Open

Add record embedding #1

fonghou opened this issue Mar 19, 2021 · 6 comments

Comments

@fonghou
Copy link

fonghou commented Mar 19, 2021

Hello, nice library!

What do you think adding record embedding like Go's struct embedding? I think it'll work nicely with optics field labels and RecordWildCards. That'll solve 99% of extensible record use cases. Handling field name conflicts may become tricky, but minimally can just generate invalid types let compiler catch them.

Thanks!

@nikita-volkov
Copy link
Owner

I'm not sure what you mean. Could you provide an example?

@fonghou
Copy link
Author

fonghou commented Mar 20, 2021

I haven't tried it. Something like this (maybe a better syntax without yaml's implicit override)

Person:
   product: &person
        name: String
        age: Int

Employee:
    product:
        << : *person
        eid: String
data Person = { name :: String, age :: Int }

data Employee = { name :: String, age :: Int, eid :: String }
 

@fonghou
Copy link
Author

fonghou commented Mar 20, 2021

Domain doesn't seem parse this (valid) yaml syntax.

declare (Just (False, True)) stdDeriver [schema|

  Person:
    product: &person
      name: String
      age: Int

  Employee:
    product:
      <<: *person
      eid: String

 |]
[1 of 1] Compiling Main

app/Main.hs:12:49: error:
    • Error at path /Employee/product/<<. Unexpected mapping value
    • In the quasi-quotation:
        [schema|

  Person:
    product: &person
      name: String
      age: Int

  Employee:
    product:
      <<: *person
      eid: String

 |]
   |
12 | declare (Just (False, True)) stdDeriver [schema|
   |                                                 ^...

@nikita-volkov
Copy link
Owner

nikita-volkov commented Mar 20, 2021

Seems like you're looking for inheritance.

While the idea does seem interesting, implementing it will introduce a major complication to the design. I believe such changes should only be done when it is proven that they solve a real pain that majority of users experience. So I suggest to let the idea age and see what other users will have to say.

@fonghou
Copy link
Author

fonghou commented Mar 21, 2021

These records behave more like structural subtyping (instead of inherence) using https://www.stackage.org/haddock/nightly-2021-03-05/generic-optics-2.0.0.0/Data-Generics-Product-Subtype.html.

Think about it be more. I agree with you it's not worth it to add such complication in template-haskell code gen phase. Some kind of yaml preprocessing just expand those anchors and references before feeding it to template-haskell would work.

Turns out yaml package Data.Yaml.decode/encode roundtrip already does that. See PR. It works beautifully for the pattern I described above.

Thanks!

@nikita-volkov
Copy link
Owner

My concern is not about implementing the thing, it is whether implementing it won't hurt the product.

The more concepts a technology introduces, the more things it requires of the newcomer to learn, thus reducing the chances of adoption. OTOH, solving a real pain of users increases those chances. So to decide whether the thing needs to be implemented we first need to prove that the feature removes a pain big enough to make the cost of complicating the techonology worthwhile.

From my experience of using "domain" it does not seem to be a pain at all. And so far you're the only one requesting to address the issue. That's why I suggest to let it age and aggregate other perspectives.


Regardless of the mentioned. If we were to implement the feature I would go for readability. E.g., something like the following:

Person:
  product:
    name: String
    age: Int

Employee:
  extends: Person
  adds:
    eid: String

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 a pull request may close this issue.

2 participants