-
Notifications
You must be signed in to change notification settings - Fork 297
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
Removing duplication from TH output #1202
Conversation
I guess CI has answered some of my questions, back to the drawing board I guess :) |
05776d5
to
4095d9a
Compare
9ac7478
to
5eba08a
Compare
Unfortunately when
As a way of getting CI ✔️ I have switched it to use the original implementation when this option is set, but this sort of defeats the point of this change, if we have to leave the old implementation in place when that option is set. I'm not sure of an alternative way around this, though it might not be such a good idea to use Another idea I had for this was to maybe generate the |
I wish we could collect metrics on when features were being used. The I suppose we could deprecate it and see if anyone complained 🤔 This feels like it should be fixable with a type annotation. All we want to do is use |
@danbroooks I'm planning on releasing |
Yeah, I've not had chance to take another look at this yet, my TH skills are not amazing either so you may need to bare with me on adding the type hints like you suggested (I have a feeling there will be an ambiguity issue still here, though happy to be wrong!). As far as merging it in as it is currently, I would say maybe not, but that is up to you. |
This reverts commit 5eba08a.
@parsonsmatt you may want to take a look at this, this latest iteration (5108cec) is maybe a good compromise between a quick fix to get TH improvements into persistent but that is relatively clean (though leaving some code duplication in place), further work could be done after this to clean things up, and I think deprecating I'm sorry but I was unable to get your typehint suggestion to work, it also adds in some more complication with regards to what the So I ended up struggling with it a bit, unfortunately! I think this might be viable still (and there is some existing functionality to do this currently in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final change - then I think we can merge this one in :)
persistent/Database/Persist/TH.hs
Outdated
|
||
fieldDefReferences :: MkPersistSettings -> EntityDef -> [FieldDef] -> Q Exp | ||
fieldDefReferences mps entDef fieldDefs = do | ||
lookupValueName "persistFieldDef" >>= \case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit dicey. Why not use the name facility lie 'persistFieldDef
? That gives you a qualified name that should work fine in the context without requiring it to be imported.
eg:
fmap ListE $ forM fieldDefs $ \fieldDef -> do
...
pure $ VarE 'persistFieldDef `AppE` fieldDefConE
Looks fantastic thanks! Ready to merge? |
Sure! 🎉 |
Before submitting your PR, check that you've:
@since
declarations to the HaddockAfter submitting your PR:
(unreleased)
on the ChangelogI'm submitting this draft PR for some initial feedback, as I think I have found a potential optimisation which drops some duplication from the resulting Haskell output. The
FieldDef
s are accessible already viapersistFieldDef
so I thought it might bring down the total code output by making that call to get at the field defs when building up theEntityDef
, rather than inlining the definitions. I'm not sure if there are implications to this so would appreciate some feedback if this is going to have negative effects, but AFAICT this is a reasonable change to make, though there may be some reason why this isn't done like this currently. I'm not sure on how much of a difference this will make to improved compile times but every little helps I suppose, and I think this will have an impact on a real world project where the are a large total number of fields.I'm not majorly keen on the implementation of
fieldDefReferences
, where it conditionally looks up thepersistFieldDef
function, specifically having to writeerror
, there might be a nicer way to do this with TH but was unable to figure it out, so resorted to the implementation currently.Below is the diff of the
dump-splices
files of thepersist-template
tests before and after this change