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

Fix JSON import #1369

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Fix JSON import #1369

merged 2 commits into from
Mar 15, 2022

Conversation

parsonsmatt
Copy link
Collaborator

Before submitting your PR, check that you've:

  • Documented new APIs with Haddock markup
  • Added @since declarations to the Haddock
  • Ran stylish-haskell on any changed files.
  • Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

dotColonE <- [|(.:)|]
dotColonQE <- [|(.:?)|]
objectE <- [|object|]
withObjectE <- [|withObject|]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bug was here. withObject was not imported, so I think this turned into an UnboundVarE instead of a VarE that we want. Wow. Using the VarE 'withObject form forces the name to be in scope, which then becomes a NameG value.

@danbroooks TIL 😅

@@ -24,7 +24,7 @@
module Database.Persist.THSpec where

import Control.Applicative (Const(..))
import Data.Aeson hiding (Key)
import Data.Aeson (decode, encode)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surfaced problem in tests by doing this

Copy link
Contributor

@danbroooks danbroooks Mar 15, 2022

Choose a reason for hiding this comment

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

Thanks for resolving this! Do you think it is worth adding a specific spec to cover imports required by users of persistent? Something like a test that only imports persistent, and defines some entities (some with derivied json instances) or do you think that might be overkill, something like:

module Database.Persist.TH.JsonEncodingSpec where

-- this test asserts this is the only import required
import Database.Persist.TH

mkPersist sqlSettings [persistLowerCase|
JsonEncoding json
    name Text
    age  Int
    Primary name
    deriving Show Eq
JsonEncoding2 json
    name Text
    age Int
    blood Text
    Primary name blood
    deriving Show Eq
JsonEncMigrationOnly json
    name Text
    age  Int
    foo  Text MigrationOnly
|]

My original PR added to JsonEncodingSpec but that was already importing Data.Aeson 😅 what you have done here seems to be a more robust solution, but I wonder if we should also add a test similar to the above? Happy to do the PR for this if so 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea.

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.

2 participants