Skip to content

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

Merged
merged 7 commits into from
Jul 5, 2022
Merged

Feat/nullable enums #1485

merged 7 commits into from
Jul 5, 2022

Conversation

sbres
Copy link
Contributor

@sbres sbres commented Mar 14, 2022

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?

@sbres
Copy link
Contributor Author

sbres commented Mar 15, 2022

I can't think of a way to add the testing of this feature.
Let me know if you have an idea.

Copy link
Collaborator

@kyleconroy kyleconroy left a 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.

@sbres
Copy link
Contributor Author

sbres commented Mar 19, 2022

I'll try to get those examples sorted out 👍

@sbres sbres force-pushed the feat/NullableEnums branch from 116177e to 6958edd Compare April 7, 2022 22:58
@sbres sbres force-pushed the feat/NullableEnums branch from 2f0e374 to db5ef4e Compare April 25, 2022 16:17
@sbres sbres requested a review from kyleconroy April 25, 2022 16:19
@sbres
Copy link
Contributor Author

sbres commented Apr 25, 2022

I've added the examples on sqlc/internal/endtoend/testdata/enum

@sbres sbres force-pushed the feat/NullableEnums branch from db5ef4e to 09e0e18 Compare April 29, 2022 09:30
@Jille Jille mentioned this pull request May 2, 2022
6 tasks
@sbres sbres force-pushed the feat/NullableEnums branch from 09e0e18 to bb5fbbf Compare May 3, 2022 13:09
@danielbprice
Copy link
Contributor

danielbprice commented May 4, 2022

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:

SELECT *
FROM sessions
WHERE
  sqlc.narg('Status')::session_status IS NULL OR status = sqlc.narg('Status')
...

I really appreciate this.

@sbres
Copy link
Contributor Author

sbres commented May 10, 2022

@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 :)

@sbres sbres force-pushed the feat/NullableEnums branch from bb5fbbf to 2054b50 Compare May 17, 2022 14:07
@tinyzimmer
Copy link
Contributor

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.

@sbres
Copy link
Contributor Author

sbres commented Jun 1, 2022

I've been trying to rebase into master for the past month.
Right now I'm a little bit short on time and unable to fix the unit test that is failing.
From what I saw It comes from the template having a new block generated just before the null enum block
Maybe it's just about re-generating the example doc in the right order? I'm not really sure at this point.
@tinyzimmer If you want to help fixing the test I'm more than happy to have a PR to the sbres:feat/NullableEnums branch

@tinyzimmer
Copy link
Contributor

I'll fork ya and see if I have any luck

@tinyzimmer
Copy link
Contributor

@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.

@tinyzimmer
Copy link
Contributor

tinyzimmer commented Jun 20, 2022

@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.

@sbres sbres force-pushed the feat/NullableEnums branch from 5548ce1 to fd56a39 Compare June 20, 2022 19:46
@sbres
Copy link
Contributor Author

sbres commented Jun 20, 2022

@tinyzimmer I've merged it the PR on the forked repo did not have the CI/CD let's see if it gets resolved

@sbres sbres force-pushed the feat/NullableEnums branch from fd56a39 to f473f52 Compare June 20, 2022 19:57
@tinyzimmer
Copy link
Contributor

@kyleconroy curious if you'll have time in the future to re-review. Assuming the test fixes are appropriate along with everything else.

@sbres
Copy link
Contributor Author

sbres commented Jul 5, 2022

@kyleconroy Can you please have a look at this, the requested changes have been done in April.
I'm coming close to delivering a project at work and I can't say "just use my forked branch".

@kyleconroy kyleconroy dismissed their stale review July 5, 2022 10:18

Outdated

@kyleconroy kyleconroy merged commit 4a0bce2 into sqlc-dev:main Jul 5, 2022
@kyleconroy
Copy link
Collaborator

@sbres Sorry this took so long to review, but I'm quite happy where it ended up.

jlisthood pushed a commit to jlisthood/sqlc that referenced this pull request Apr 28, 2023
* 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>
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.

4 participants