Optionally derive PathMultiPiece for composite keys
#1509
Conversation
There was a problem hiding this comment.
I don't think I'm in favor of this change. There's a bit too much special-case logic here.
Currently, there are two ways of deriving instances for keys: the mps setting, and manually.
mkPersist sqlSettings [persistLowerCase|
Blah
name String
|]
instance PathMultiPiece (Key Blah) where ...I'm OK with introducing features to make manual instances easier - ie exporting the function defaults that are based on PersistEntity a => ..., and a newtype for DerivingVia.
| -- | An auxiliary class to enable 'PathMultiPiece' instance derivation for | ||
| -- 'PersistEntity' composite keys. Instances of 'PersistPathMultiPiece' should | ||
| -- be derived using the @DeriveAnyClass@ strategy. | ||
| -- | ||
| -- @since 2.14.6.0 | ||
| class PersistEntity record => PersistPathMultiPiece record where | ||
| -- | 'fromPathMultiPiece' wrapper. The default implementation uses | ||
| -- 'keyFromValues' and 'fromPathPiece', which are provided by the context. | ||
| keyFromPieces :: [Text] -> Maybe (Key record) | ||
| keyFromPieces pieces = do | ||
| Right key <- keyFromValues <$> mapM fromPathPiece pieces | ||
| pure key | ||
| -- | 'toPathMultiPiece' wrapper. The default implementation uses | ||
| -- 'keyToValues' and 'toPathPiece', which are provided by the context. | ||
| keyToPieces :: Key record -> [Text] | ||
| keyToPieces = map toPathPiece . keyToValues |
There was a problem hiding this comment.
Why is this class required? I think that we should be able to inline the definition directly into the PathMultiPiece (Key record) instance.
| instance PersistPathMultiPiece record => PathMultiPiece (Key record) where | ||
| fromPathMultiPiece = keyFromPieces | ||
| toPathMultiPiece = keyToPieces |
There was a problem hiding this comment.
So PersistPathMultiPiece is used to provide custom behavior to the PathMultiPiece instance. Instead of writing,
mkPersist sqlSettings [persistLowerCase| ... |]
instance PathMultiPiece (Key MyRecord) where
fromPathMultiPiece = keyFromPieces
toPathMultiPiece = keyToPiecesWe write:
mkPersist sqlSettings [persistLowerCase| ... |]
instance PersistPathMultiPiece MyRecordI think I am a little hesitant on this approach. The more idiomatic approach is to use a newtype wrapper for use with DerivingVia, like so:
newtype ViaPersistEntity a = ViaPersistEntity (Key a)
instance (PersistEntity a) => PathMultiPiece (Key a) where
fromPathMultiPiece = ...
toPathMultiPiece = ....
-- Use site:
mkPersist sqlSettings [persistLowerCase| ... |]
deriving via ViaPersistEntity MyRecord instance PathMultiPiece (Key MyRecord)There was a problem hiding this comment.
I've replaced PersistPathMultiPiece with the more idiomatic approach in 950098c.
persistent/Database/Persist/Quasi.hs
Outdated
|
|
||
| Primary firstPart secondPart | ||
|
|
||
| deriving PathMultiPiece |
There was a problem hiding this comment.
This usage is definitely something I'd consider a problem - the deriving clause is for the datatype, not the key, and I don't want to start introducing special-case logic to decide if a class is intended for a key or the datatype (or both).
There was a problem hiding this comment.
This usage, and the Haddock change documenting it here, have been removed in 950098c.
persistent/Database/Persist/TH.hs
Outdated
| | typeclass == ''PathMultiPiece = | ||
| ViaStrategy $ ConT ''ViaPersistEntity `AppT` recordType |
There was a problem hiding this comment.
Apologies, I should have been more clear - I'm not going to accept a PR that has a special-case for how an instance is derived.
| | typeclass == ''PathMultiPiece = | |
| ViaStrategy $ ConT ''ViaPersistEntity `AppT` recordType |
There was a problem hiding this comment.
OK, so regarding your first comment,
I'm OK with introducing features to make manual instances easier - ie exporting the function defaults that are based on
PersistEntity a => ..., and anewtypeforDerivingVia.
should I revert my changes to Database.Persist.TH and remove CompositeKeyPathMultiPieceSpec? That would leave ViaPersistEntity and its PathMultiPiece instance as the additions from this PR. If those don't seem useful on their own, I'm fine with closing the PR.
I'll keep the rest on my fork as automatic PathMultiPiece derivation is helpful for my specific use case, although I don't think it would be used often if it was added here (no issues have been raised to request it).
There was a problem hiding this comment.
Yes, that works.
I need to document more about how to hook into the derivation stuff without modifying this function here.
The way that I would approach this is with an additional function that operates on the [UnboundEntityDef] from a thing. So, you have,
$(share [mkPersist sqlSettings, derivePathMultiPiece] ...)Where derivePathMultiPiece :: [UnboundEntityDef] -> Q [Dec], and you can do something like:
derivePathMultiPiece =
concat <$> traverse derivePathMultiPieceFor
where
derivePathMultiPieceFor unboundEntityDef =
let keyTypeName = ...
[d| deriving via (ViaPersistentEntity $(keyTypeName)) instance PathMultiPiece $(keyTypeName) |]This gives you the same automatic instance generation, without modifying the upstream code.
This PR enables
PathMultiPieceinstance derivation for entities with composite keys usingmpsDeriveInstances.Unlike
PathPiece, an instance is not derived automatically. This should avoid the coupling concerns expressed in #649 and #1458. A similar technique could be used forPathPieceetc. in a future (breaking) change to address these issues.Before submitting your PR, check that you've:
@sincedeclarations to the Haddockpersistentversion2.14.6.0.stylish-haskellon any changed files.stylish-haskellsuggests fixes toDatabase.Persist.Class.PersistEntitywhich are unrelated to this PR: Tidy language extensions and imports in PersistEntity module blujupiter32/persistent#10.editorconfigfile for details)After submitting your PR:
(unreleased)on the Changelog