-
Notifications
You must be signed in to change notification settings - Fork 301
JSON addition to Database.Persist.Postgresql #793
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
Conversation
… and new Filter operators (@>.) and (<@.)
MaxGabriel
left a comment
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.
Thanks for writing such detailed documentation, strong 👍. I had a few comments:
- The Haddocks aren't rendering how I think you want them to
- Could you update the
persistent-postgresql/Changelog.mdfile with a link to your PR? - Would it be possible to write a test for this? It could be scoped to just Postgres using the preprocessor
ifdefs the test suite already uses. This isn't essential but would be quite nice
| -- null @> 23 --> False | ||
| -- null @> "null" --> False | ||
| -- null @> {} --> False | ||
| -- @ |
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.
| -- Any empty Object matches any object | ||
| -- @ | ||
| -- {"a":1,"b":false} @> {} --> True | ||
| -- @ |
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 documentation has quite a few examples, and I think if formatted correctly would be pretty huge. My default would be to hide them in an ==== __Examples__ collapsible section, but it might be OK since this module is quite small
| (@>.) :: EntityField record Value -> Value -> Filter record | ||
| (@>.) field val = Filter field (Left val) $ BackendSpecificFilter " @> " | ||
|
|
||
| -- | Same as `(<@.)` except the inclusion check is reversed. |
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.
I think you want single quotes, not backticks, for that
| case eitherDecodeStrict bs of | ||
| Left str -> Left $ fromPersistValueParseError "FromJSON" bs $ T.pack str | ||
| Right v -> Right v | ||
| fromPersistValueJsonB x = Left $ fromPersistValueError "FromJSON" "string or bytestring" x |
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.
Instead of bytestring, could you use a database name like Postgres' bytea?
* Upgraded the randomValues function in DataTestTypes.hs to actually have random values everytime * small OCD
…work because of it
|
Updated a lot. Should also pass all tests. |
|
Thanks for your work on this! Great patch; really like the documentation with all the examples, and the copious tests. I've merged and released on Hackage 👍 |

Added
PersistFieldandPersistFieldSqlinstances forData.Aeson.Valueand newFilteroperators(@>.)and(<@.)that work withValue.