-
Notifications
You must be signed in to change notification settings - Fork 804
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
Conversation
Number: argn, | ||
Location: fun.Location, | ||
}) | ||
} |
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.
aside: encapsulating the positional tracking into named.ParamSet
significantly cleaned up this code.
@@ -54,25 +54,31 @@ func Mutate(raw string, a []Edit) (string, error) { | |||
if len(a) == 0 { |
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.
I was having a bit of trouble with this, so I tested the heck out of it and these were the resulting edits.
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.
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 I went ahead and implemented |
@@ -54,25 +54,31 @@ func Mutate(raw string, a []Edit) (string, error) { | |||
if len(a) == 0 { |
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.
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.
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. |
ah, I didn't look hard enough for the docs. But yes I can do that. |
Added here: #1579 |
Just kidding, one that actually passes CI here: #1580 |
EDITED from original PR after discussion here: #1525
Exec Summary
I implemented user-specified nullability with
sqlc.narg
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).