feat: Add with_params_from method to clients query request builder
#24927
Conversation
There was a problem hiding this comment.
Hey @jbajic! Thanks for tackling this issue. One thing before I approve the change. Can you add a test to this same file to test that the functionality works? There should be tests you can use as an example in the file. If you need any help let me know!
Done |
|
@jbajic Two changes so I can actually merge this that has nothing to do with your code: can you run |
jbajic
force-pushed
the
add-with-params-to-client
branch
from
743a130
to
a12dbf6
Compare
jbajic
force-pushed
the
add-with-params-to-client
branch
from
a12dbf6
to
651d84c
Compare
influxdb3_client/src/lib.rs
Outdated
| /// let client = Client::new("http://localhost:8181")?; | ||
| /// let response_bytes = client | ||
| /// .api_v3_query_sql("db_name", "SELECT * FROM foo WHERE bar = $bar AND foo > $fooz") | ||
| /// .with_params_from(HashMap::from([ |
There was a problem hiding this comment.
Is the HashMap necessary here? Or can you just pass in the array, i.e.,
.with_params_from([
("bar", json!(false)),
("foo", json!(10)),
])There was a problem hiding this comment.
We can pass any iterable collection with pairs that fit the traits. I will replace it with array
influxdb3_client/src/lib.rs
Outdated
| /// ``` | ||
| pub fn with_params_from<S, P, C>(mut self, params: C) -> Result<Self> | ||
| where | ||
| S: Into<String> + Clone, |
There was a problem hiding this comment.
I wonder if we can remove the Clone bound on this, since .clone() is only called on name after it has been converted to a String (which implements Clone). If it can be removed here it could also likely be removed on the with_try_params method.
There was a problem hiding this comment.
You are right it can be removed, and it will be.
influxdb3_client/src/lib.rs
Outdated
|
|
||
| let r = client | ||
| .api_v3_query_influxql(db, query) | ||
| .with_params_from(Vec::from([ |
There was a problem hiding this comment.
Similar here, is the Vec needed? or can you just use the array [...] directly.
There was a problem hiding this comment.
LGTM - thank you @jbajic for contributing. @mgattozzi may want to take another look before we merge.
Closes #24812
Changes
Added method
with_params_fromto theQueryRequestBuilder.