Skip to content

Allow args to driver's raw Exec#886

Merged
brandur merged 1 commit into
masterfrom
brandur-exec-args
May 9, 2025
Merged

Allow args to driver's raw Exec#886
brandur merged 1 commit into
masterfrom
brandur-exec-args

Conversation

@brandur

@brandur brandur commented May 8, 2025

Copy link
Copy Markdown
Contributor

Currently, unlike underlying connections/transactions, the driver
version of Exec doesn't take parameters. This is mostly okay because
Exec is mainly needed for migrations, but as I was trying to convert
some code over in Pro's test suite, I did find that it'd be kind of nice
to be able to use parameters there too so we can use an executor instead
of raw transaction.

This is probably okay because the executor version of QueryRow already
lets parameters be passed in. There is a possible problem wherein
different databases use different placeholders (Postgres's $ versus
SQLite's ? for example), but when we solve that we'll have to solve it
for QueryRow anyway.

Currently, unlike underlying connections/transactions, the driver
version of `Exec` doesn't take parameters. This is mostly okay because
`Exec` is mainly needed for migrations, but as I was trying to convert
some code over in Pro's test suite, I did find that it'd be kind of nice
to be able to use parameters there too so we can use an executor instead
of raw transaction.

This is probably okay because the executor version of `QueryRow` already
lets parameters be passed in. There is a possible problem wherein
different databases use different placeholders (Postgres's `$` versus
SQLite's `?` for example), but when we solve that we'll have to solve it
for `QueryRow` anyway.
@brandur brandur requested a review from bgentry May 8, 2025 03:27
@brandur brandur merged commit 1d04eb0 into master May 9, 2025
10 checks passed
@brandur brandur deleted the brandur-exec-args branch May 9, 2025 00:13
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.

2 participants