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

Use stock deriving strategy on generated Show instances for Uniques #1068

Closed

Conversation

cdparks
Copy link

@cdparks cdparks commented Apr 2, 2020

Hey we ran into this at Freckle trying to turn on -Wmissing-deriving-strategies with ghc 8.8.3. Currently, we're seeing warnings when compiling Entity declarations with a Unique, e.g.:

mkPersist sqlSettings [persistUpperCase|
  Thing
    foo Text
    UniqueThingFoo foo
    deriving Show Eq
|]

  <snip>/persistent/persistent-template/test/main.hs:44:1: warning: [-Wmissing-deriving-strategies]
      No deriving strategy specified. Did you want stock, newtype, or anyclass?
     |
  44 | mkPersist sqlSettings [persistUpperCase|
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

Specifying the stock strategy for the Unique's generated Show instance silences this warning:

 data instance Unique Thing = UniqueThingFoo Text
-  deriving Show
+  deriving stock Show

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)

ghc 8.8.3 with -Wmissing-deriving-strategies enabled emits a warning
when compiling an Entity declaration with a Unique:

  mkPersist sqlSettings [persistUpperCase|
    Thing
      foo Text
      UniqueThingFoo foo
      deriving Show Eq
  |]

  <snip>/persistent/persistent-template/test/main.hs:44:1: warning: [-Wmissing-deriving-strategies]
      No deriving strategy specified. Did you want stock, newtype, or anyclass?
     |
  44 | mkPersist sqlSettings [persistUpperCase|
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

Specifying the stock strategy for the Unique's Show instance silences
the warning:

    data instance Unique Thing = UniqueThingFoo Text
  -   deriving Show
  +   deriving stock Show
@parsonsmatt
Copy link
Collaborator

Current plan is to get rid of Show actually - but I'd really like to make that customizable. Perhaps we need a mpsUniqueDerives field?

@parsonsmatt
Copy link
Collaborator

We can definitely get this done for the persistent-2.10 branch to fix the immediate issue.

@cdparks
Copy link
Author

cdparks commented Apr 2, 2020

get this done for the persistent-2.10 branch

Adding mpsUniqueDerives or this PR?

@parsonsmatt
Copy link
Collaborator

This PR, for sure, mpsUniqueDerives is a new field and thus a breaking change, so would need to go in for 2.11.

@parsonsmatt parsonsmatt changed the base branch from master to persistent-2.10 April 2, 2020 17:03
@parsonsmatt parsonsmatt changed the base branch from persistent-2.10 to master April 2, 2020 17:04
@parsonsmatt
Copy link
Collaborator

hmmm, I think there's gotta be a better way to just apply that one commit to the old branch than setting the base branch, which tries to get everything in

@cdparks
Copy link
Author

cdparks commented Apr 2, 2020

Are you trying to just pick faa44fd16 and make persistent-2.10 the base? I can do some creative rebasing and force-push to my branch if you just want the code change minus version/changelog stuff

@cdparks
Copy link
Author

cdparks commented Apr 2, 2020

(and by creative rebasing, I just mean dropping the top commit, and modifying the first one to not bump a version, so I guess not especially creative)

@parsonsmatt
Copy link
Collaborator

I think I just want to cherry pick that commit onto the persistent-2.10 branch

@cdparks
Copy link
Author

cdparks commented Apr 2, 2020

Cool, let me know if you need me to fiddle with anything.

@chessai
Copy link

chessai commented May 30, 2020

Don't forget Eq!

+1

@cdparks
Copy link
Author

cdparks commented Nov 4, 2020

@parsonsmatt is this subsumed by #1127? If so, I'm fine to close this

@parsonsmatt
Copy link
Collaborator

If you'd like to make a backported release with this patch, we can do that. But otherwise, yeah, since we're not generated Show instances anymore we don't need this. Sorry for not getting back to it 😅

@parsonsmatt parsonsmatt closed this Nov 4, 2020
@cdparks
Copy link
Author

cdparks commented Nov 4, 2020

Not a problem, we usually upgrade to the most recent version of persistent pretty quickly

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

Successfully merging this pull request may close these issues.

3 participants