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

API to construct InputTypes for Record types. #530

Merged

Conversation

eamsden
Copy link

@eamsden eamsden commented Jul 30, 2018

Adds the RecordInputType type and associated functions inputFieldWith, inputField, and inputRecord.

These inputField{,With} functions and the Contravariant and Divisible operations create RecordInputType values which describe how a Haskell record type may be encoded into a Dhall expression. The inputRecord function converts a RecordInputType to an InputType.

An example (using the same type as the example for RecordType) is given in the documentation comment for `RecordInputType.

See #525

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Looks great! Just two small comments

src/Dhall.hs Outdated
> injectProject :: InputType Project
> injectProject =
> inputRecord
> ( adapt >$< inputField "name" (inject @Text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the type applications are not necessary since they should be inferred by the context

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right. And actually, given the way the API is written, they should either be inputFieldWith or the second parameter should be dropped entirely. The example as written woudn't typecheck!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we have doctest enabled on this module, so if you transform the example into a doctest then it will be checked 🙂

Copy link
Author

Choose a reason for hiding this comment

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

I might do that in a separate PR, as it would involve modifying the $setup block to bring the Project type into scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's not urgent to do in this PR

src/Dhall.hs Outdated
-}

-- | Infix 'contramap'
(>$<) :: Contravariant f => (a -> b) -> f b -> f a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that the >$< operator already exists in the contravariant package:

http://hackage.haskell.org/package/contravariant-1.5/docs/Data-Functor-Contravariant.html

... so we can reuse that

Copy link
Author

Choose a reason for hiding this comment

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

I can drop that definitiion and just rexport the contravariant definition. Give me a few minutes.

@quasicomputational
Copy link
Collaborator

Can I bikeshed the naming here? We already have an input* family of functions that reads expressions from strings in various ways; at a glance, because of the naming, these look like members of that family, but they're not, really. So maybe recordFieldWith, recordField and recordInputType would be better?

@eamsden
Copy link
Author

eamsden commented Jul 30, 2018

@quasicomputational maybe we should use the inject prefix? I was naming after InputType, which they are intended to aid construction of.

calling them recordField{,With} doesn't really give an idea of their distinction from field

src/Dhall.hs Outdated
(>*<) :: Divisible f => f a -> f b -> f (a, b)
(>*<) = divided

infixr 4 >*<
Copy link
Author

Choose a reason for hiding this comment

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

I think this should be infixl 4 >*< to work properly with the >$< exported from contravariant, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should still be infixr if you want the tuple nesting to be (a, (b, c)), but the precedence needs to be elevated to 5

@eamsden
Copy link
Author

eamsden commented Aug 6, 2018

@Gabriel439 does this need anything else before it can be merged?

@Gabriella439
Copy link
Collaborator

@eamsden: No, nothing needs to be done. I just thought you had additional changes planned, but if you think this is ready then I'll go ahead and merge

@Gabriella439 Gabriella439 merged commit 23e15f5 into dhall-lang:master Aug 6, 2018
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.

None yet

3 participants