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

Record fields list #117

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Sep 23, 2021

Following the alias -> record rename #116 this PR aims to make record representation more accurate

Support for {} records on elm side (these are very important in elm as they're core part of extensible records). This type of record is never derived by generics instance but it can be defined by hand. This makes representation of ElmRecord closer to real elm record which might become handy in the future.

In JSON, empty record is represented by [] to make implementation compatible with aeson deriving.

Comment on lines +218 to +221
-- Even though elm supports records with no field {} we don't ever derive these
-- Haskell records are in fact just type field accessors so there is ambiguity for zero fields constructors
-- Usually it makes more sense to derive Single constructor type for the Elm side so that's what we do
-- However it's possible to manually define instance of Elm class that would produce empty records (the other unit) type on Elm side
Copy link
Member Author

Choose a reason for hiding this comment

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

Important comment

Comment on lines +96 to +100

encodeRecordUnit : T.RecordUnit -> Value
encodeRecordUnit x = list (\_ -> null) []
Copy link
Member Author

@turboMaCk turboMaCk Sep 23, 2021

Choose a reason for hiding this comment

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

decodeStrict @RecordUnit "[]" 
Just RecordUnit

Comment on lines +102 to +105
decodeRecordUnit : Decoder T.RecordUnit
decodeRecordUnit = D.succeed {}
Copy link
Member Author

@turboMaCk turboMaCk Sep 23, 2021

Choose a reason for hiding this comment

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

toJSON $ RecordUnit
Array []

in theory we can fail here unless json contents empty array. Not if there will be any advantage though.

Comment on lines +81 to +84
<> if isEmptyRecord
then emptyRecordDecoder
else if elmRecordIsNewtype
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really excited about this. Any idea?

@turboMaCk turboMaCk marked this pull request as ready for review September 24, 2021 07:50
@turboMaCk turboMaCk requested a review from jhrcek September 24, 2021 07:50
Comment on lines +158 to +170
data RecordUnit = RecordUnit
deriving stock (Generic, Eq, Show)
deriving anyclass (FromJSON, ToJSON)

instance Elm RecordUnit where
toElmDefinition _ = DefRecord $ ElmRecord
{ elmRecordName = "RecordUnit"
, elmRecordFields = []
, elmRecordIsNewtype = False
}

Copy link
Member Author

Choose a reason for hiding this comment

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

manual instance implementation of record with no fields.

@jhrcek
Copy link
Contributor

jhrcek commented Sep 24, 2021

I'm not very fond of this change.
Is there any actual use case for empty records that you envision we'll be using (as opposed to try to tightly support all what elm allows)?
Can you give a small example of how you'd take advantage of this on the elm side?

@turboMaCk
Copy link
Member Author

turboMaCk commented Sep 24, 2021

I think it's better to represent elm types close to how elm represents it. Modeling record fields as non empty values is simply wrong. I assume one of the reasons was for support of elmRecordIsNewtype (which is also not producing accurate representation of newtype in Elm). Regular records are sort of compatible with empty records out of the box.

The other advantage is that list is simpler to define than nonempty list so this also removes a bunch of noise from manual declaration of elm record (which I think is especially annoying since it should not be required).

Where this might be handy later is if we get to same feature where we would start utilizing elm's extensible records for one reason or the other. I outlined one of the possible use-case in #45 (comment). I don't expect it to be something we implement but this change would be required for it as well as any other feature that would manipulate records in similar way.

I don't think there is any nice elm code I can show because in the end one can always do Json.Decode.map (always {}) generatedDecoder already anyway. This is not as much about of what one can do on elm side as it is about what elm-street could generate and how user can define Elm instances.

Try to think about it. I know it might seem contra intuitive to see advantage of adding a special case but I think in long run this should lead to elimination of special cases. Especially if we get to implementing truly unboxable and type safe newtype generation.

@turboMaCk turboMaCk marked this pull request as draft September 24, 2021 10:43
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.

2 participants