-
Notifications
You must be signed in to change notification settings - Fork 883
Add emit_result_struct_pointers and emit_params_struct_pointers options #979
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
Conversation
…ns for Go generator
Would love to see this landed soon! It'd make the generated types much more idiomatic: https://github.com/golang/go/wiki/CodeReviewComments#receiver-type |
@kyleconroy anything we can do to help get this merged? Using pointer types would be a lot more idiomatic imo. |
@dchertkov I'm so sorry reviewing this pull request fell through the cracks. If you're willing to fix the merge conflicts and update to the latest code, I can promise to review it as soon as it's ready. Let me know. |
@kyleconroy All is ready. |
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.
Overall this approach looks good. A few things to clean up before we can get this merged.
internal/endtoend/testdata/emit_result_and_params_struct_pointers/query.sql
Outdated
Show resolved
Hide resolved
SELECT a, b FROM foo | ||
` | ||
|
||
func (q *Queries) GetAll(ctx context.Context) ([]*Foo, error) { |
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 slice of structs seems weird to me, but I guess that's what you'd want in this case?
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.
There has been some discussion about performance https://stackoverflow.com/questions/27622083/slices-of-structs-vs-slices-of-pointers-to-structs
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.
We'll need to update the docs with the new configuration options, but that can be done in a separate PR.
Add emit_result_struct_pointers and emit_params_struct_pointers options for Go generator.
This is second attempt at improvement as noted here #812