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

Derive Generic instances for non-generic newtype keys #990

Merged

Conversation

jessekempf
Copy link
Contributor

@jessekempf jessekempf commented Nov 26, 2019

Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

--
-- Default: []
--
-- @since 2.7.4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh hell yes

@@ -902,6 +912,8 @@ mkKeyTypeDec mps t = do
useNewtype = pkNewtype mps t
customKeyType = not (defaultIdType t) || not useNewtype || isJust (entityPrimary t)

supplement :: [Name] -> [Name]
supplement names = names <> (filter (`notElem` names) $ mpsDeriveInstances mps)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like these somewhat awkward elem dances could be made easier by a gentle use of Set


import Database.Persist
import Database.Persist.TH
import TemplateTestImports


share [mkPersist sqlSettings { mpsGeneric = False }, mkDeleteCascade sqlSettings { mpsGeneric = False }] [persistUpperCase|
share [mkPersist sqlSettings { mpsGeneric = False, mpsDeriveInstances = [''Generic] }, mkDeleteCascade sqlSettings { mpsGeneric = False }] [persistUpperCase|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it, awesome.

else do
let allInstances = [''Show, ''Read, ''Eq, ''Ord, ''PathPiece, ''ToHttpApiData, ''FromHttpApiData, ''PersistField, ''PersistFieldSql, ''ToJSON, ''FromJSON]
let allInstances = supplement [''Show, ''Read, ''Eq, ''Ord, ''PathPiece, ''ToHttpApiData, ''FromHttpApiData, ''PersistField, ''PersistFieldSql, ''ToJSON, ''FromJSON]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point for future upgrade: remove the unnecessary instances here like PathPiece, ToHttpApiData, etc that don't make sense for non-web-apps.

@parsonsmatt parsonsmatt added this to the template-2.8.1 milestone Jan 28, 2020
@parsonsmatt parsonsmatt merged commit 0cb5c45 into yesodweb:master Jan 28, 2020
@parsonsmatt
Copy link
Collaborator

Released in persistent-template-2.8.1, thanks so much! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants