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 Inject instance for tuples #100

Merged
merged 1 commit into from Aug 21, 2017

Conversation

Projects
None yet
2 participants
@bosu
Collaborator

bosu commented Aug 20, 2017

(a, b) is mapped to { _1 = a, _2 = b }

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Aug 20, 2017

Collaborator

What I'd suggest doing here is to first define an R2 type, like this:

data R2 a b = R2 { _1 :: a, _2 :: b }
    deriving (Generic, Inject, Interpret)

... and then defining the Inject instance for the tuple by taking advantage of the Contravariant instance for InputType:

instance (Inject a, Inject b) => Inject (a, b) where
    injectWith = fmap (contraMap adapt) injectWith
      where
        adapt (_1, _2) = R2 {..})

The advantage of this approach is that it's easy to scale out to larger tuple sizes

The other advantage is that you can also export the R{N} family of constructors so that users can also use those directly instead of tuples if they want the correspondence to the Dhall type to be more clear

Collaborator

Gabriel439 commented Aug 20, 2017

What I'd suggest doing here is to first define an R2 type, like this:

data R2 a b = R2 { _1 :: a, _2 :: b }
    deriving (Generic, Inject, Interpret)

... and then defining the Inject instance for the tuple by taking advantage of the Contravariant instance for InputType:

instance (Inject a, Inject b) => Inject (a, b) where
    injectWith = fmap (contraMap adapt) injectWith
      where
        adapt (_1, _2) = R2 {..})

The advantage of this approach is that it's easy to scale out to larger tuple sizes

The other advantage is that you can also export the R{N} family of constructors so that users can also use those directly instead of tuples if they want the correspondence to the Dhall type to be more clear

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Aug 20, 2017

Collaborator

Also, while you're doing this, you can also define a matching Interpret instance for tuples, too

Collaborator

Gabriel439 commented Aug 20, 2017

Also, while you're doing this, you can also define a matching Interpret instance for tuples, too

@bosu

This comment has been minimized.

Show comment
Hide comment
@bosu

bosu Aug 20, 2017

Collaborator

What I'd suggest doing here is to first define an R2 type, like this:

You are right, this looks much better. I'll update the pull request.

Also, while you're doing this, you can also define a matching Interpret instance for tuples, too

Will do.

Collaborator

bosu commented Aug 20, 2017

What I'd suggest doing here is to first define an R2 type, like this:

You are right, this looks much better. I'll update the pull request.

Also, while you're doing this, you can also define a matching Interpret instance for tuples, too

Will do.

@bosu

This comment has been minimized.

Show comment
Hide comment
@bosu

bosu Aug 20, 2017

Collaborator

Rebased and updated the pull request.

Collaborator

bosu commented Aug 20, 2017

Rebased and updated the pull request.

@Gabriel439

It looks like the build failure is not your fault. Somehow the ansi-terminal-0.7 release has confused cabal's solver. I'm still digging into exactly why

Show outdated Hide outdated src/Dhall.hs Outdated
Add Inject, Interpret instances for tuples.
Uses R2 intermediate type to represent tuples in Dhall.
@bosu

This comment has been minimized.

Show comment
Hide comment
@bosu

bosu Aug 21, 2017

Collaborator

Update pull request uses DeriveAnyClass

Collaborator

bosu commented Aug 21, 2017

Update pull request uses DeriveAnyClass

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Aug 21, 2017

Collaborator

I'll go ahead and merge this and then fix the build failure on master separately since this pull request is not the cause of the problem

Collaborator

Gabriel439 commented Aug 21, 2017

I'll go ahead and merge this and then fix the build failure on master separately since this pull request is not the cause of the problem

@Gabriel439 Gabriel439 merged commit fa51d5e into dhall-lang:master Aug 21, 2017

1 check failed

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

@bosu bosu deleted the bosu:tuple-it branch Aug 21, 2017

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