search

lpsmith / postgresql-simple

Mid-level client library for accessing PostgreSQL from Haskell
Other
206 stars 71 forks source link

Added HStoreV2 & related tests. #214

Open saurabhnanda opened 7 years ago

saurabhnanda commented 7 years ago

Fixes #204

HStore has been copy-pasted and modified to create HStoreV2. There is a lot of scope for V2 to reuse functions & types defined in V1.

saurabhnanda commented 7 years ago

@lpsmith This is not ready to be merged right now (too much code duplication), but we would appreciate if you could skim through the code and point out any glaring errors.

/cc @sras

saurabhnanda commented 7 years ago

@lpsmith also, is there any way to allow the users of pg-simple to pick between two versions of HStoreList and HStoreMap -- one for (Text, Text) and the other for (Text, Maybe Text) without needing to have two separate version of HStore itself?

saurabhnanda commented 7 years ago

@lpsmith is the following a good idea preserve backward compatibility AND to give the users of the library a choice to pick between Text or Maybe Text as the type of the hstore values:

newtype HStoreBase a = HStoreList {fromHStoreList :: [(Text, a)]} deriving (Show)
type HStoreList = HStoreBase Text
type HStoreNullable = HStoreBase (Maybe Text)
lpsmith commented 7 years ago

@lpsmith also, is there any way to allow the users of pg-simple to pick between two versions of HStoreList and HStoreMap -- one for (Text, Text) and the other for (Text, Maybe Text) without needing to have two separate version of HStore itself?

I dunno, I haven't given that idea much thought myself. The idea with HStoreBase is plausible, though I don't have any immediate reaction as to whether or not it would be a great idea going forward. (Although, it might make sense to do some additional conversions, e.g. to integers or whatever, though this would be a terribly inefficient way to store a map of text to integers in postgres.)

Though the one thing that strikes me about your statements is that you either misspoke, or you have the relationship between the two modules wrong. It would seem to me that there is a lot of opportunity for the HStore module to re-use code in the HStoreV2 module, the relationship in reverse (as you literally wrote) seems a little backwards to me.

saurabhnanda commented 6 years ago

@lpsmith did you get a change to decide on this?

saurabhnanda commented 6 years ago

@lpsmith any thoughts on this or #215 ? A PR in Opaleye's repo is waiting for this upstream merge.

lpsmith commented 6 years ago

Sorry, life's been pretty crazy for me the last year, and I've not been paying enough attention to my open source efforts. So I do apologize for ignoring this issue for so long; you aren't the only one.