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

Configure positional parameter logic #1498

Closed
kyleconroy opened this issue Mar 19, 2022 · 10 comments
Closed

Configure positional parameter logic #1498

kyleconroy opened this issue Mar 19, 2022 · 10 comments
Labels
accepted enhancement New feature or request good first issue Good for newcomers

Comments

@kyleconroy
Copy link
Collaborator

kyleconroy commented Mar 19, 2022

What do you want to change?

For Go, sqlc will use a positional argument for a query if there's a single parameter. If there's more than one, it will generate a parameters struct. While this works for me, a few other folks would like different behavior.

@brentd would like to use positional arguments for up to three parameters (#1496)
@Threpio would like to always use parameter structs (#1373)

How should we do it?

We can solve this using a single configuration parameter called query_parameter_limit, which defaults to 1 (the current behavior). We can set the value to a pointer in the configuration YAML / JSON so that zero is a valid option. Anything less than zero should be an error.

What programming language backends need to be changed?

Go

@kyleconroy kyleconroy added enhancement New feature or request triage New issues that hasn't been reviewed good first issue Good for newcomers labels Mar 19, 2022
@kyleconroy kyleconroy removed the triage New issues that hasn't been reviewed label Mar 19, 2022
@iAziz786
Copy link

I am very new to sqlc, given I put some research in it I would like to pick this.

@skabbes
Copy link
Contributor

skabbes commented May 1, 2022

Just to chime in - I really like the positional argument for only 1 parameter - I would even like positional arguments for more parameters (maybe up to 3?), but only if their types are distinct. That's what I would like out of a Go generator.

When there are more than 3 parameters, then a struct is more ergonomic anyway. But when there are <= 3 parameters, of all distinct types, the compiler will protect you from transposing them, and it should be self-documenting enough.

Crazy Idea
Another kind of bizzare pattern that could be applied would be to apply the functional options pattern(But applied to arguments, not options). The boilerplate to facilitate it is rather tedious, but it would be generated anyway. Here's what I'd be shooting for (in user-written code)

_, _ := queries.CreateAuthor(ctx,
  db.Name("Stephen King"),
  db.Bio(sql.NullString{Valid: true, String: "wrote books"}))
  
// or maybe by using some better naming for the package names
_, _ := queries.CreateAuthor(ctx,
  params.Name("Stephen King"),
  params.Bio(sql.NullString{Valid: true, String: "wrote books"}))

This has a few side effects:

  1. You would get full compile-time safety. Forgot a parameter? compile error. Reordered parameters? compile error (but easy to fix, or maybe make them alphabetical?)
  2. It would be slower than it is now, but still significantly faster than any reflection-based one
  3. It is self-documenting at the call site.
  4. You have to write the parameters in exactly the right order :(. But I am not sure how to get around that without sacrificing compile safety.

Would anyone be interested in something like that?

And here is a little template for what I think would be required to do that.
Generated Boilerplate

// we start with private structs
type createAuthorParams struct {
	name string
	bio  sql.NullString
}

func (c *createAuthorParams) setName(name string) {
	c.name = name
}

func (c *createAuthorParams) setBio(bio sql.NullString) {
	c.bio = bio
}


type nameParam = func (nameSetter)

type nameSetter interface {
	setName(string)
}

type bioParam = func (bioSetter)

type bioSetter interface {
	setBio(sql.NullString)
}

func Bio(b sql.NullString) bioParam {
	return func (s bioSetter) {
		s.setBio(b)
	}
}

func Name(name string) nameParam {
	return func (s nameSetter) {
		s.setName(name)
	}
}

func (q *Queries) CreateAuthor(ctx context.Context, name nameParam, bio bioParam) (sql.Result, error) {
	var p createAuthorParams
	name(&p)
	bio(&p)
	return q.db.ExecContext(ctx, createAuthor, p.name, p.bio)
}

@farazfazli
Copy link

I think having the option be configurable would be a great advantage. Reason being - sometimes we want to decode JSON directly into the struct emitted by sqlc, and in that case having the option to set it to 0 would be quite helpful.

@Threpio
Copy link
Contributor

Threpio commented Jun 13, 2022

Hi @kyleconroy - Is this still open or has it been resolved?

Happy to pick this up and code this as work has given me a bit of time to do open-source work.

@kyleconroy
Copy link
Collaborator Author

@skabbes That's an interesting proposal, but a bit beyond what this issue is tackling. Want to split it out into a separate proposal?

@Threpio Yes, this is still open. There are a few open PRs that I need to review.

@skabbes
Copy link
Contributor

skabbes commented Aug 29, 2022

Since the time I wrote the proposal, I think I learned a lot more about my own use cases - and also the thing others are struggling with - with regard to sqlc.

I (personally) think that "sqlc core engine" (parser, "typechecking", etc) could use some love before trying to double-down on anything code-gen related. I think I'll "retract" my proposal, so I can help out on those areas first. I'd really want to avoid 'death by 1000 paper cuts', and I don't think my proposal is really limiting anyone from using sqlc - it would just be a nice to have.

@farazfazli
Copy link

Hey @kyleconroy - any update on this?

@go-mez
Copy link
Contributor

go-mez commented Apr 7, 2023

@farazfazli the first proposal is already merged

@farazfazli
Copy link

@go-mez That's awesome! Good job.

@kyleconroy
Copy link
Collaborator Author

query_parameter_limit is now live :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants