-
Notifications
You must be signed in to change notification settings - Fork 86
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
with_param #159
base: main
Are you sure you want to change the base?
with_param #159
Conversation
8891fb7
to
32382c9
Compare
edbd788
to
abf96dd
Compare
Aside from the simpler types, the following needs testing:
I'd maybe also not mix it with |
So already found issue with basic tests. String Note once a string is contained it is expected to be quoted in params: ClickHouse/clickhouse-connect#159 |
@@ -160,6 +161,12 @@ impl Client { | |||
self | |||
} | |||
|
|||
pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { |
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.
What's the purpose of Client::with_param
? Parameters are specific for queries, not the whole client
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.
Generally, but if someone wants to share parameter between queries queries they can put it on the client
For example, in a multi tenant app, tenantid could be put as a parameter in the function creating client
We also need tests for And need to add some examples. |
@@ -195,6 +195,12 @@ impl Query { | |||
self.client.add_option(name, value); | |||
self | |||
} | |||
|
|||
pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { |
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.
I'm still trying to figure out the naming here. This one is consistent with with_option
, but that name emphasizes the connection with Client::with_option
.
For instance, bind
mimics sqlx bind.
A simple param()
would be more concise. Yep, it's inconsistent for now, but once Client::builder()
is introduced, we can have fn option()
instead of with_option
.
@@ -195,6 +195,12 @@ impl Query { | |||
self.client.add_option(name, value); | |||
self | |||
} | |||
|
|||
pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { |
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.
Result<>
can be very inconvenient. It can be used in a concise way only inside -> Result
functions (q.with_param("a", 42)?.with_param("b", 43)?
), but sometimes you want explicitly handle errors with matching inside the function that doesn't return Result
, so ?
(via the Try
trait) cannot be used.
So, I suggest delayed errors in the same way as in the bind()
method.
@@ -195,6 +195,12 @@ impl Query { | |||
self.client.add_option(name, value); | |||
self | |||
} | |||
|
|||
pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { |
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.
This method must be well documented. Also, we should specify the difference between param()
and bind()
(probably in a dedicated section on Query
and provide links to that section in docs of both methods).
Small wrapper around
with_option
forimpl Serialize + Bind