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

UPDATED: sqlc.narg() - allow user-specified nullability #1536

Merged
merged 12 commits into from
Apr 30, 2022

Conversation

skabbes
Copy link
Contributor

@skabbes skabbes commented Apr 9, 2022

EDITED from original PR after discussion here: #1525

Exec Summary
I implemented user-specified nullability with sqlc.narg

sqlc.narg('param') -- regardless of whether this parameter was inferred to be nullable or not, it will be nullable in the generated code.

Details
Most of the work here came from building types to represent set of named parameters, which tracks the name and nullability status of any parameter (inferred or user defined).

named.Param
This type can track its nullability from the sqlc engine (inferred nullability) separately from any user-specified nullability, and always prefers the user-specified version to the inferred one.

named.ParamSet
This type is essentially a collection of params, and helps build up parameters, and encapsulate their positional representations as well. As you can see - this type significantly cleaned up compiler/resolve.go

Next Steps
This code works, is tested. I think a code review would be great, and maybe a pointer to how to update the docs (or maybe the doc repos are still private).

Number: argn,
Location: fun.Location,
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside: encapsulating the positional tracking into named.ParamSet significantly cleaned up this code.

@skabbes skabbes changed the title sqlc.arg() with user-defineable nullability UPDATED: sqlc.narg() - allow user-specified nullability Apr 30, 2022
@@ -54,25 +54,31 @@ func Mutate(raw string, a []Edit) (string, error) {
if len(a) == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having a bit of trouble with this, so I tested the heck out of it and these were the resulting edits.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The edit code is super hacky so I'm not surprised you needed to add all these tests. Hopeful in the future we can get rid of all of this code and generate SQL using an AST.

@skabbes
Copy link
Contributor Author

skabbes commented Apr 30, 2022

@kyleconroy I went ahead and implemented sqlc.narg like we discussed, could you take a look at you earliest convenience?

@@ -54,25 +54,31 @@ func Mutate(raw string, a []Edit) (string, error) {
if len(a) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The edit code is super hacky so I'm not surprised you needed to add all these tests. Hopeful in the future we can get rid of all of this code and generate SQL using an AST.

@kyleconroy kyleconroy merged commit a22a7b7 into sqlc-dev:main Apr 30, 2022
@skabbes skabbes deleted the sqlc_arg_nullable branch April 30, 2022 22:13
@kyleconroy
Copy link
Collaborator

It's a lot to ask, but can you also add docs to https://docs.sqlc.dev/en/stable/howto/named_parameters.html in a separate PR? Want to make sure that people know this option exists.

@skabbes
Copy link
Contributor Author

skabbes commented Apr 30, 2022

ah, I didn't look hard enough for the docs. But yes I can do that.

@skabbes
Copy link
Contributor Author

skabbes commented Apr 30, 2022

Added here: #1579

@skabbes
Copy link
Contributor Author

skabbes commented May 1, 2022

Just kidding, one that actually passes CI here: #1580

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.

2 participants