Skip to content

Allow for mixing parameter styles #816

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

Closed
wants to merge 4 commits into from

Conversation

ovadbar
Copy link
Contributor

@ovadbar ovadbar commented Nov 27, 2020

This should allow for mixed styles of parameters.

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.

Thanks for the patch! I agree that it doesn't make much sense to require a single parameter style. A few request:

  • please pick a more descriptive name for the tests, such as sqlc_arg_mixed or params_mixed.
  • Move the current tests into a postgresql subdirectory and add the same test for MySQL

@ovadbar
Copy link
Contributor Author

ovadbar commented Nov 29, 2020

Thanks for the feedback.

(One thing I did notice is if you have a query as follows, the numbers always precede the sqlc.args convetion)

SELECT count(1) FROM table_name WHERE id > $1 AND id < sqlc.arg(id);

@dant4301
Copy link

This would be great fix and would be awesome if merged.

There is an outstanding issue w.r.t using named_parameter for LIMIT for MySQL. We can use this fix to sort of work around it in the short term, but for the below:

I applied the patch and tried it for the query:

SELECT * FROM books WHERE book_id >= sqlc.arg(min_book_id) ORDER BY title ASC LIMIT ?;

The generated code looks like this:

type GetBooksByNameParams struct {
        Limit                 int32 `json:"limit"`
        MinBookID int64 `json:"min_book_id"`
}

func (q *Queries) GetBooksByName(ctx context.Context, arg GetBooksByNameParams) ([]Category, error) {
        rows, err := q.query(ctx, q.getBooksByNameStmt, getCategoriesByName, arg.Limit, arg.MinBookID)
...

Aren't the orders of parameters arg.Limit and arg.MinBookID switched?

@ovadbar
Copy link
Contributor Author

ovadbar commented Jun 27, 2021

Yeah I kind of realized that was an issue but, with postgresql you can use dollar numbers format so it is not an issue and I did not think of how it would work with mysql.

I can look into fixing it but I really do not have the time.

@kyleconroy
Copy link
Collaborator

I can look into fixing it but I really do not have the time.

Understandable. I also don't have the time to look into fixing this. @dant4301 If you'd like this feature, you're going to have to get your hands dirty and take a shot at fixing this PR. I'm going to close this for now, but feel free to fork this branch and open up another PR if you can get it to work.

@kyleconroy kyleconroy closed this Jun 28, 2021
@ovadbar
Copy link
Contributor Author

ovadbar commented Jun 30, 2021

@dant4301 can you look at #1071 and see if the new pull request solves your problems

@dant4301
Copy link

@ovadbar, I do not use sqlc now since I did a limited amount of codegen just enough for my needs. Thanks again though.

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.

3 participants