-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement UUID Column Type #28
Conversation
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.
Looks good on a first glance, so if @arybczak is also happy I think we can merge this.
hpqtypes-extras.cabal
Outdated
@@ -109,4 +110,5 @@ test-suite hpqtypes-extras-tests | |||
tasty, | |||
tasty-hunit, | |||
text, | |||
transformers | |||
transformers, | |||
uuid |
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.
It looks like this isn't needed.
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.
UUID
is needed in the tests. I have removed the dependency in main and updated the tests to use uuid-types
.
LGTM apart from redundant dependency, thanks! We'll make a release once fields-json 0.4.0.0 sits on hackage. Can you bump fields-json dependency to |
Done |
4d31e32
to
8168bf1
Compare
I force-pushed the branch with fixed release date. |
This PR adds a new
UuidT
column type which makes use of the UUID format implemented in scrive/hpqtypes#17.It also modifies the existing test to make use of the UUID type instead of regular integer. The alternative is to have a mix of INT and UUID when testing some of the tables. Otherwise I can write separate tests just for UUID.
This change is