-
Notifications
You must be signed in to change notification settings - Fork 883
Feat/nullable enums #1485
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
Feat/nullable enums #1485
Conversation
I can't think of a way to add the testing of this feature. |
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.
To test this, we need to add or update the existing test queries in internal/endtoend
. We should write a SELECT, INSERT, UPDATE, and DELETE query for both MySQL and PostgreSQL to verify that the null types are used correctly.
I'll try to get those examples sorted out 👍 |
116177e
to
6958edd
Compare
2f0e374
to
db5ef4e
Compare
I've added the examples on sqlc/internal/endtoend/testdata/enum |
db5ef4e
to
09e0e18
Compare
Please excuse the unsolicited comment but: Thanks for doing this! I hit exactly this problem tonight when testing the new sqlc.narg() functionality with an enum type. I pulled your branch, built it, and I can confirm that your fix seemed to cleanly solve this problem for my query, which looks like this:
I really appreciate this. |
@danielbprice Thanks for your comment, I got scared that there was a bug on the first two sentences hahaha. @kyleconroy Do you think you can have a look at this PR? The changes you requested have been done :) |
Also recently ran into this issue and looking forward to this PR. If there is anything I can do to contribute (code-wise or testing) let me know. |
I've been trying to rebase into master for the past month. |
I'll fork ya and see if I have any luck |
@sbres I've fixed the failing test on a PR into your branch, but I'm unsure if that's all the issue was or not. |
@sbres Have you a chance to look at my PR? I'm still not sure if it's sufficient, but would love to see this get merged. |
- We need to import driver to comply with the Valuer interface. - The Scan method uses the Scan from the Non Null Enum type
- Regen with the Null types
5548ce1
to
fd56a39
Compare
@tinyzimmer I've merged it the PR on the forked repo did not have the CI/CD let's see if it gets resolved |
fd56a39
to
f473f52
Compare
@kyleconroy curious if you'll have time in the future to re-review. Assuming the test fixes are appropriate along with everything else. |
@kyleconroy Can you please have a look at this, the requested changes have been done in April. |
@sbres Sorry this took so long to review, but I'm quite happy where it ended up. |
* Generate Null Enum type - We need to import driver to comply with the Valuer interface. - The Scan method uses the Scan from the Non Null Enum type * Map Custom Null enum type with database typing function * regen examples and internal endtoend tests - Regen with the Null types * Add Enum examples with nullable fields * Add NullableEnum output to e2e test * Update generated files with correct version . Co-authored-by: Stephane <sbres@coinshares.com> Co-authored-by: Avi Zimmerman <avi.zimmerman@gmail.com>
With this PR now custom Enums have a nullable structure like all the sql.NullXXXX
The Null structure is generated for every Enum.
When enums are present on the model we import
database/sql/driver
The database type matching is a little bit dirty I think something better can be done, let me know if you have an idea.
Also, how should I go about creating tests for this?