Skip to content

Conversation

@Vlix
Copy link
Contributor

@Vlix Vlix commented Mar 1, 2018

Added PersistField and PersistFieldSql instances for Data.Aeson.Value and new Filter operators (@>.) and (<@.) that work with Value.

Copy link
Member

@MaxGabriel MaxGabriel left a 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.md file 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
-- @
Copy link
Member

Choose a reason for hiding this comment

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

I don' think these haddocks are rendering how you want them to

image

-- Any empty Object matches any object
-- @
-- {"a":1,"b":false} @> {} --> True
-- @
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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?

@Vlix
Copy link
Contributor Author

Vlix commented Mar 12, 2018

Updated a lot. Should also pass all tests.

@MaxGabriel MaxGabriel merged commit 2bed4f8 into yesodweb:master Mar 12, 2018
@MaxGabriel
Copy link
Member

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 👍

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