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

Rework TH instances context #1133

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevladimir
Copy link

Instead of deriving instances for all datatype variables iterate over constructors generating instances based on how variables are used. Does the right thing in cases like

  • Phantom type variables
    Does not add corresponding instance to the context for unused variables
  • Type families
    Instead of generating (ToJSON a) for data Foo a = Foo (SomeTypeFamily a) generates ToJSON (SomeTypeFamily a)

Note that it slightly changes the behavior in cases like

data Foo a = Foo a deriving (ToJSON)

data Bar a = Bar (Foo a)
deriveToJSON defaultOptions ''Bar

will result in ToJSON (Foo a) constraint rather than ToJSON a (current behavior). I believe the former is more precise as that is exactly what generated encoder wants. The small downside is requirement for FlexibleContext extension.

TODO and open questions:

  • How to test TH?
    Are there any helpers to get easy access to instance context. I think it would be nice to have something like
$(getContext $ deriveToJSON ...) `shouldBe` "ToJSON a, ToJSON b"

Can add if required. But maybe there are already established approach to test that.

  • Which test cases to add?
    I sketched up some cases, but that list is not exhaustive. And they should be reworked according to previous point
  • Should we test both From and To classes?
  • Add Changelog

Does the right thing in various tricky cases like phantom type variables and
type families
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.

1 participant