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

From/ToDhall no longer takes InterpretOptions argument #1696

Merged

Conversation

akrmn
Copy link
Collaborator

@akrmn akrmn commented Mar 4, 2020

Coming from aeson, I wanted to define FromDhall instances for my types which built on GHC.Generics but with tweaked options (say, drop prefixes). Ideally, I would write something like

{- dhall type: { Name : Text, Email : Text }
-}
data Person = Person
  { personName :: Text
  , personEmail :: Text
  } 
  deriving (Generic)

instance FromDhall Person where
  autoWith = genericAutoWith defaultInterpretOptions 
    { fieldModifier = Text.drop (Text.length "person") 
    }

The problem with this is that autoWith itself takes a InterpretOptions argument, which is set to defaultInterpretOptions by auto, the most common entry point. This means that the only way to enforce a canonical FromDhall instance for a type is by ignoring the argument to autoWith, which is confusing for callers. In practice, this PR is only reflecting in the types the fact that all instances of FromDhall and ToDhall necessarily ignore the InterpretOptions, either because they don't need it or because they overwrite it. The only field that was actually used, inputNormalizer, was kept as an input.

  • InterpretOptions split into smaller InterpretOptions record and InputNormalizer newtype.
  • FromDhall method autoWith and ToDhall method injectWith no longer take InterpretOptions arguments, instead, they take an InputNormalizer, since the only field that instances of these classes ever used was inputNormalizer (only used by instances for (->)).
  • GenericFromDhall method genericAutoWithNormalizer (renamed from genericAutoWith) and GenericToDhall method genericToDhallWithNormalizer(renamed from genericToDhallWith) now take an InputNormalizer in addition to an InterpretOptions.

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.

Yeah, I agree that the aeson way of doing things makes more sense! Thank you for fixing this 🙂

@Gabriella439 Gabriella439 merged commit ac13627 into dhall-lang:master Mar 6, 2020
@akrmn
Copy link
Collaborator Author

akrmn commented Mar 6, 2020

Awesome! Thanks a lot, Gabriel

@Gabriella439
Copy link
Collaborator

@akrmn: You're welcome! 🙂

@patrickmn
Copy link
Collaborator

@akrmn Catching a codebase that used a custom fieldModifier up with this change, and it does not appear that defaultInputNormalizer is exported, which I need for autoWith (right?)

@Gabriella439
Copy link
Collaborator

@patrickmn: Yeah, it's not currently exported, but it's not hard to locally define:

https://hackage.haskell.org/package/dhall-1.31.0/docs/src/Dhall.html#defaultInputNormalizer

@patrickmn
Copy link
Collaborator

@Gabriel439 for that I'd need the newtype to be exported, right?

Or, in other words, how would I externally make an instance with a custom field modifier? (I can't use DerivingVia.)

@Gabriella439
Copy link
Collaborator

@Gabriella439
Copy link
Collaborator

@patrickmn: Oh sorry, wrong newtype. Never mind

Gabriella439 added a commit that referenced this pull request Mar 30, 2020
@Gabriella439
Copy link
Collaborator

@patrickmn: I have a fix up here: #1727

I can also cut a 1.31.1 point release for this

@patrickmn
Copy link
Collaborator

Awesome, will finish my port as soon as it's merged just in case there's anything else pre-1.31.1.

mergify bot added a commit that referenced this pull request Mar 30, 2020
* Expose `{default,}InputNormalizer`

... as caught by @patrickmn in #1696 (comment)

* Fix missing haddocks

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@patrickmn
Copy link
Collaborator

All good. Thank you!

@Gabriella439
Copy link
Collaborator

@patrickmn: You're welcome! 🙂

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.

3 participants