Visitar URL original
Rename Type to Decoder, InputType to Encoder by sjakobi · Pull Request #1485 · dhall-lang/dhall-haskell · GitHub
Skip to content

Rename Type to Decoder, InputType to Encoder - #1485

Merged
sjakobi merged 9 commits into
masterfrom
sjakobi/1393-decoder-encoder
Oct 30, 2019
Merged

sjakobi merged 9 commits into
masterfrom
sjakobi/1393-decoder-encoder

Conversation

@sjakobi

@sjakobi sjakobi commented Oct 28, 2019

Copy link
Copy Markdown
Collaborator

… as a part of #1393.

Comment thread dhall/src/Dhall.hs Outdated
, UnionEncoder(..)
, encodeConstructorWith
, encodeConstructor
, encodeUnion

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it was good to get rid of the "input*" names here, but I'm not convinced about these "encode*" names.

Comment thread dhall/src/Dhall.hs
, recordEncoder
, UnionEncoder(..)
, encodeConstructorWith
, encodeConstructor

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm still not sure about the encode{Field,Constructor}With names. It's really a bit unfair that the FromDhall side got to use field and constructor!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Would constructorEncoder[With] and fieldEncoder[With] be any better? At least it would keep the encode name free for better uses…

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We could also consider creating separate modules for encoding and decoding. Then we could have field[With] and constructor[With], and union and record in both modules.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To decrease the unfairness, how about names like to{Record, Field, Singleton}With for the ToDhall side and from{Record, Field, Singleton}With for FromDhall?

About module for encoding and decoding, it's an out-there suggestion but, the way tomland encodes and decodes toml is with a bi-directional codec, something like

data Codec a = 
    Codec { fromDhall  :: Expr Src Void -> Extractor Src Void a
            toDhall :: a -> Expr Src Void
            dhallType :: Expr Src Void
            }

It's pretty nice to keep the symmetry, and this way you always have to implement both encoders and decoders at the same time.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

{to,from}Record don't seem too helpful to me since we have records on both sides. Similarly for {to,from}Field.

The Codec idea is intriguing, but this PR is not the right place for discussing it IMHO!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let's maybe leave the lack of symmetry between record and union decoders and encoders for future work.

@sjakobi
sjakobi merged commit d5b7c38 into master Oct 30, 2019
@sjakobi
sjakobi deleted the sjakobi/1393-decoder-encoder branch October 30, 2019 13:24
sjakobi added a commit that referenced this pull request Oct 30, 2019
sjakobi added a commit that referenced this pull request Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants