feat(Go):Add query_parameter_limit conf to codegen #1558
Conversation
go-mez
force-pushed
the
feat/query-parameter-limit
branch
from
04ab48f
to
f7c492c
Compare
|
@kyleconroy sorry, now its passing all the tests, i think... I was not aware of the dev guide (https://github.com/kyleconroy/sqlc/blob/main/docs/guides/development.md)... i was just running |
docs/reference/config.md
Outdated
| @@ -78,6 +79,8 @@ Each package document has the following keys: | |||
| - Customize the name of the querier file. Defaults to `querier.go`. | |||
| - `output_files_suffix`: | |||
| - If specified the suffix will be added to the name of the generated files. | |||
| - `query_parameter_limit`: | |||
| - Positional arguments that will be generated in go functions (>= `1` or `-1`). To always emit a parameter struct, you would need to set it to `-1`. `0` is invalid. Defaults to `1`. | |||
There was a problem hiding this comment.
I changed my mind here. We should allow for zero to be a valid value. Anything less than zero should be an error.
|
@go-mez Sorry for taking so long to get you a review. I know you've resolved conflicts a few times. I can promise that you'll only need to do it one more time. Thanks again for understanding. |
|
Any chance, to merge this to master? I could help if needed. |
|
Not sure what's happened, but the tests haven't run for this PR. Maybe another rebase and push will fix things? |
|
@go-mez any chance to get this rebased and pushed? seems like only the docs were waiting... Would be cool to have that feature |
|
@polderudo i'll do it. dont worry! |
|
@kyleconroy i think it is okay now. could you approve the workflows please ? |
Thanks man! I didn't want to push however, just asked because we are at the start of a project and that would save a lot of refactoring. |
|
@kyleconroy could you please take a look at the PR and approve the workflows? |
There was a problem hiding this comment.
I left a few comments scattered about.
I really like this (and I would set the parameter to 3 or 4 myself), with one caveat.
I would personally prefer a struct in any case (no matter how few params) in which all the parameter types aren't distinct. That is, these are all good function signatures:
func SomeQuery(ctx context.Context, id int32)
func SomeQuery(ctx context.Context, id int32, s string)
func SomeQuery(ctx context.Context, id int32, s string, created time.Time, x SomeEnum, b bool)But this one isn't:
func SomeQuery(ctx context.Context, id1 int32, id2 int32)Even though it has only two params, they have the same type, which makes it easy to accidentally transpose them.
(It would be even better if each model's ID automatically had its own type, like type UserID int32, but that's a different topic. That can be achieved manually as needed.)
internal/endtoend/testdata/process_plugin_sqlc_gen_json/gen/codegen.json
Show resolved
Hide resolved
internal/endtoend/testdata/process_plugin_sqlc_gen_json/gen/codegen.json
Outdated
Show resolved
Hide resolved
|
@josharian thank you for the review and the suggestion. i will try to address it |
There was a problem hiding this comment.
lgtm, modulo on little comment and my larger design comment above.
guess it's back to @kyleconroy
|
ping @kyleconroy |
kyleconroy
force-pushed
the
feat/query-parameter-limit
branch
from
0a12f6b
to
6154e2f
Compare
|
thankyou guys! |
|
Works nice. Thanks for the work, although there is a issue with pgx. |
|
Did you find any time to look into this go-mez? |
|
@polderudo do you know if this error was happening before? ill try to check it anytime soon. could you also provide an example in the playground ? (https://play.sqlc.dev/) |
|
It's working, if i remove the new config Here is a palyground: |
|
ping. any update? |
Related to: #1498.
I'm not sure about the approach I took, so just let me know if im solving it wrongly