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

Interpret instance for unnamed data fields #103

Merged
merged 1 commit into from Aug 22, 2017

Conversation

Projects
None yet
2 participants
@bosu
Collaborator

bosu commented Aug 21, 2017

Maps unnamed data fields to the positional record keys. For example:

data Foo = Foo Bool Integer Natural deriving (Generic, Interpret)

is mapped to:

Record { _1 : Bool, _2 : Integer, _3 : Natural }

Dhall data type.

@Gabriel439

I think this is a great idea! If we do this then we can eliminate the need for R2. Just a few comments

Show outdated Hide outdated src/Dhall.hs Outdated
Show outdated Hide outdated src/Dhall.hs Outdated
@@ -505,20 +506,34 @@ defaultInterpretOptions = InterpretOptions
for automatically deriving a generic implementation
-}
class GenericInterpret f where
genericAutoWith :: InterpretOptions -> Type (f a)
genericAutoWith :: InterpretOptions -> State Int (Type (f a))

This comment has been minimized.

@Gabriel439

Gabriel439 Aug 22, 2017

Collaborator

Since we're turning genericAutoWith into a monadic function, we can also automatically thread the InterpretOptions through the code by changing the type to:

genericAutoWith :: ReaderT InterpretOptions (State Int (Type (f a)))

... although it might be slightly easier to work with this type:

genericAutoWith :: StateT Int (Reader InterpretOptions (Type (fa)))
@Gabriel439

Gabriel439 Aug 22, 2017

Collaborator

Since we're turning genericAutoWith into a monadic function, we can also automatically thread the InterpretOptions through the code by changing the type to:

genericAutoWith :: ReaderT InterpretOptions (State Int (Type (f a)))

... although it might be slightly easier to work with this type:

genericAutoWith :: StateT Int (Reader InterpretOptions (Type (fa)))

This comment has been minimized.

@bosu

bosu Aug 22, 2017

Collaborator

The amended commit much simplified the diff. IMHO Reader may be unnecessary anymore...

@bosu

bosu Aug 22, 2017

Collaborator

The amended commit much simplified the diff. IMHO Reader may be unnecessary anymore...

{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeApplications #-}

This comment has been minimized.

@Gabriel439

Gabriel439 Aug 22, 2017

Collaborator

Note that TypeApplications only works for GHC 8.0 and later, but the Travis test suite still supports GHC 7.8 (in spirit; the test is currently failing due to cabal solver issues)

However, I'd be willing to drop GHC 7.8 support because even Debian stable now supports GHC 8.0

@Gabriel439

Gabriel439 Aug 22, 2017

Collaborator

Note that TypeApplications only works for GHC 8.0 and later, but the Travis test suite still supports GHC 7.8 (in spirit; the test is currently failing due to cabal solver issues)

However, I'd be willing to drop GHC 7.8 support because even Debian stable now supports GHC 8.0

This comment has been minimized.

@bosu

bosu Aug 22, 2017

Collaborator

+1 for dropping GHC 7.8

@bosu

bosu Aug 22, 2017

Collaborator

+1 for dropping GHC 7.8

Show outdated Hide outdated src/Dhall.hs Outdated
Show outdated Hide outdated tests/Regression.hs Outdated
Show outdated Hide outdated dhall.cabal Outdated
Interpret instance for unnamed data fields
Maps unnamed data fields to the positional record keys. For example:

data Foo = Foo Bool Integer Natural deriving (Generic, Interpret)

is mapped to:

Record { _1 : Bool, _2 : Integer, _3 : Natural }

Dhall data type.
@bosu

This comment has been minimized.

Show comment
Hide comment
@bosu

bosu Aug 22, 2017

Collaborator

Sorry for losing the remarks by using amended push request. All of them were addressed. Now I see the advantages of additional commits :)

Regarding R2 removal: it is still needed by Inject instance... Probably the same kind of refactoring can happen there.

Collaborator

bosu commented Aug 22, 2017

Sorry for losing the remarks by using amended push request. All of them were addressed. Now I see the advantages of additional commits :)

Regarding R2 removal: it is still needed by Inject instance... Probably the same kind of refactoring can happen there.

@Gabriel439 Gabriel439 merged commit b581007 into dhall-lang:master Aug 22, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Aug 22, 2017

Collaborator

Thanks! :)

Collaborator

Gabriel439 commented Aug 22, 2017

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment