Add generic machinery for Record instances #52
Conversation
|
I had to do some module restructuring to avoid cyclic imports. I am happy to walk those changes back and just move everything to one module, or whatever you think is best for project structuring. This was just my initial take! |
scripts/shell.nix
Outdated
| @@ -6,6 +6,8 @@ in with pkgs; | |||
| cabal-install | |||
| hlint | |||
| haskellPackages.apply-refact | |||
| haskellPackages.fourmolu | |||
There was a problem hiding this comment.
is it possible to specify the version? We're using v0.12 in the CI, it wouldn't be nice for people to land on another version :(
There was a problem hiding this comment.
Would you mind if I changed the shell.nix to a flake. Its really hard to control the dependency versions using (it just uses the nixpkgs that the users system has in scope, instead of a project specific pin).
| import Data.Text.Display.Core | ||
| import Data.Text.Display.Generic |
There was a problem hiding this comment.
How does this translate in terms of haddocks? Will people see barebones re-exports page?
There was a problem hiding this comment.
forget what I wrote, you fix this at the top it seems.
There was a problem hiding this comment.
I think so, but the documentation should be preserved on the module specific pages.
src/Data/Text/Display/Generic.hs
Outdated
| -- | We leverage the `Generic.Data.GenericProduct` type to prevent consumers | ||
| -- from deriving instances for sum types. Sum types should use a manual instance | ||
| -- or derive one via `ShowInstance`. | ||
| -- | ||
| -- @since 0.0.5.0 | ||
| instance (Generic a, GDisplay1 (Rep a)) => Display (GenericProduct a) where |
There was a problem hiding this comment.
I don't know generic-data at all, what is the error message when you try to derive Display for a Sum?
There was a problem hiding this comment.
This is where it happens https://hackage.haskell.org/package/generic-data-1.1.0.0/docs/Generic-Data-Internal-Error.html#t:AssertNoSum. I agree that its not the best error message, but I was going to have to write some annoying type family machinery to get the appropriate custom error message, so I figured this was a fine interim solution.
There was a problem hiding this comment.
I see. This might seem petty but I'm not too chuffed about the fact that generic-data transitively depends on array, stm, StateVar and contravariant.
How annoying would be the tyfam solution? More annoying than what was in the pg-entity code?
There was a problem hiding this comment.
Let me see. I think it actually shouldn't be too bad.
There was a problem hiding this comment.
Why do you not want to derive Display for sums? The GDisplay instance for (:+:) is fine, isn't it? As far as I can tell, this really is a better fit for Generically, which is defined in base recently.
GenericProduct on its own doesn't do anything different from Generically. It was meant to address a strange corner case of generic deriving for Monoid. If you want to forbid generic deriving on sums, it doesn't matter whether the type is named Generically or GenericProduct, you have to add the relevant error message to the (:+:) instance.
There was a problem hiding this comment.
I'm open to Generically deriving the instances for Sums. I was just imagining that we wanted to constrain the way that some of the deriving via machinery was used. I actually just cribbed the AssertSum bit and placed it on the RecordInstance newtype.
src/Data/Text/Display/Generic.hs
Outdated
|
|
||
| -- | | ||
| -- Module : Data.Text.Display.Generic | ||
| -- Copyright : © Hécate Moonlight, 2021 |
There was a problem hiding this comment.
I believe you can put your name for the copyright :)
There was a problem hiding this comment.
Ah, I wasn't sure what to do there!
| -- > • 🚫 Cannot derive Display instance for MySum via RecordInstance due to sum type | ||
| -- > 💡 Sum types should use a manual instance or derive one via ShowInstance. | ||
| -- > • When deriving the instance for (Display MySum) |
|
@JonathanLorimer would you mind rebasing on top of HEAD? :) |
JonathanLorimer
force-pushed
the
jonathan/generic-record-machinery
branch
from
ba52155
to
e6a92d0
Compare
|
@JonathanLorimer I'll let you fix the hlint errors and I'll merge. Thanks a bunch for this PR! |
Submitter checklist