Skip to content
This repository was archived by the owner on Mar 25, 2021. It is now read-only.

Add a newtype implementing generic instances for use with deriving via #37

Closed
wants to merge 1 commit into from
Closed

Add a newtype implementing generic instances for use with deriving via #37

wants to merge 1 commit into from
+162 −109

Conversation

kl0tl
Copy link
Contributor

@kl0tl kl0tl commented Apr 14, 2020

I went with the name suggested by @hdgarrood in https://github.com/purescript/purescript-generics-rep/issues/36#issuecomment-613196483. There’s also the cute Generically, used for the same purpose in generic-data if we want to follow Haskell.

Instances must be defined in the same module than the newtype to avoid orphans but every Data.Generic.Rep.* module import Data.Generic.Rep so I had to move the Generic class under a new Data.Generic.Rep.Class module to break circular dependencies.

This depends on a few changes on the compiler:

diff --git i/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs w/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs
index 612a06a9..63a6f06d 100755
--- i/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs
+++ w/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs
@@ -292,7 +292,7 @@ verifySuperclasses ss mn env className tys derivingStrategy =
             else tell . errorMessage' ss $ UnverifiableSuperclassInstance derivingStrategy constraintClass className tys
 
 dataGenericRep :: ModuleName
-dataGenericRep = ModuleName [ ProperName "Data", ProperName "Generic", ProperName "Rep" ]
+dataGenericRep = ModuleName [ ProperName "Data", ProperName "Generic", ProperName "Rep", ProperName "Class" ]
 
 dataEq :: ModuleName
 dataEq = ModuleName [ ProperName "Data", ProperName "Eq" ]

An alternative would be to not export the newtype from Data.Generic.Rep and require users to import Data.Generic.Rep.Derive directly.

Close https://github.com/purescript/purescript-generics-rep/issues/36.

@hdgarrood
Copy link
Contributor

I’d definitely not want to move the class and require a compiler patch; that’s going to make this change much more painful, as then any given version of this library can then only work with compilers pre- or post- whatever version we released that as, and additionally it’s going to give awful error messages if you do mix them up. Requiring users to import a separate Derive module is much more appealing to me.

Can I request that we close this until deriving via has landed in the compiler, though?

@kl0tl
Add a newtype implementing generic instances for use with deriving via

Verified

This commit was signed with the committer’s verified signature.
edolstra Eelco Dolstra
GPG key ID: 8170B4726D7198DE
Verified
Learn about vigilant mode
7978985
@kl0tl kl0tl force-pushed the derive-via-newtype branch from 7d43c29 to 7978985 Compare April 15, 2020 18:25
@kl0tl
Copy link
Contributor Author

kl0tl commented Apr 15, 2020

I didn’t think about the consequences of such compiler patch, sorry about that. I moved the class back to Data.Generic.Rep.

Can I request that we close this until deriving via has landed in the compiler, though?

Let’s talk about this later then!

@kl0tl kl0tl closed this Apr 15, 2020
@hdgarrood
Copy link
Contributor

Not a problem! This is what code review is for :)

@kl0tl kl0tl mentioned this pull request Dec 26, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[generics-rep] Newtypes for deriving via
2 participants
@kl0tl @hdgarrood