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

Added HStoreV2 & related tests. #214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saurabhnanda
Copy link

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.

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

@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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Owner

lpsmith commented Jun 10, 2017

@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
Copy link
Author

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

@saurabhnanda
Copy link
Author

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

@lpsmith
Copy link
Owner

lpsmith commented Nov 9, 2017

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.

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.

None yet

2 participants