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

Add support for emitting pointer types for nullable columns #1571

Merged
merged 2 commits into from
Nov 10, 2022
Merged

Add support for emitting pointer types for nullable columns #1571

merged 2 commits into from
Nov 10, 2022

Conversation

nickbruun
Copy link
Contributor

pgx/v4 supports passing and scanning into pointer types for nullable columns instead of using the standard library Null<Type> representation, which is super handy if you wind up interfacing a lot with, say, generated Protocol Buffer files using the proto3 optional flag.

This PR adds a new configuration option emit_pointers_for_null_types, which, as per the documentation line added:

If true and sql_package is set to pgx/v4, generated types for nullable types are emitted as pointers (ie. *string) instead of database/sql null types (ie. NullString). Defaults to false.

This change would be a significant quality-of-life improvement for me, but I obviously don't know if it generally fits with the philosophy of sqlc, so consider this a suggestion and please feel free to provide any feedback (including, but not limited to bike-shedding of the choice of name for the configuration parameter, if we see this feature fit for sqlc.)

Lastly, thanks all for building this tool. It's genuinely saved me tons of hours writing the same pesky boiler plate with limited guarantees.

@adoublef
Copy link

how can I try this out? I have been looking at the alternative which is to manually override but that is pretty tedious. would like to try this out just don't know how to include it.

@mvrhov
Copy link

mvrhov commented May 26, 2022

This was implemented before (named a bit differently), but it has been removed. #814 has a list of needed overrides. Which is a bit tedious to copy around I'd say.

@adoublef
Copy link

@mvrhov so is it just recommend to add them manually then I'm guessing?

Thanks for this list as it does help.

@mvrhov
Copy link

mvrhov commented May 27, 2022

For now I guess. I do like this PR because then I don't have to copy a bunch of lines into each project.

@nickbruun
Copy link
Contributor Author

nickbruun commented Jun 4, 2022

Certainly avoiding having to copying a bunch of lines around would be a huge improvement. Any chance this could be considered for a merge in your eyes, despite prior history, @mvrhov?

@mvrhov
Copy link

mvrhov commented Jun 7, 2022

@nickbruun It's not my decision on what's getting merged.

@nickbruun
Copy link
Contributor Author

@mvrhov I understand – mostly just wondering what historical context you can provide since it sounds like this was tried before?

@kyleconroy
Copy link
Collaborator

@nickbruun Sorry that I'm just getting around to getting this reviewed. I know it's been months since it was opened.

I'm onboard with this change and would like to get it merged for the pgx/v5 release #1823

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.

How should this work in conjunction with the new pgtype package? I think we should only return pointers to non-pgtype types. Thoughts?

@nickbruun
Copy link
Contributor Author

Let me review the changes from pgx/v5 and I'm happy to update in accordance :)

@brlala
Copy link

brlala commented Oct 17, 2022

would be good to have this feature in v5

@JamesArthurHolland
Copy link

Is this going to be merged soon? I'm using sqlc, gqlgen and gqlgenc, getting an error about String is incompatible with database/sql.NullString and I'd much rather use the pointers for null types.

@kyleconroy kyleconroy added this to the v1.17.0 milestone Nov 10, 2022
@kyleconroy kyleconroy merged commit 27fffbf into sqlc-dev:main Nov 10, 2022
akutschera pushed a commit to akutschera/sqlc that referenced this pull request Nov 10, 2022
@nickbruun
Copy link
Contributor Author

Oh wow, I totally got behind on developing a coherent view on pgx/v5. Thanks for merging this!

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.

6 participants