Pager supporting QueryValues and consistency #337
Conversation
src/cluster/pager.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn query<Q>(&'a mut self, query: Q) -> QueryPager<'a, Q, SessionPager<'a, M, S, T>> | ||
| pub fn query<Q>(&'a mut self, query: Q, qv: Option<QueryValues>) -> QueryPager<'a, Q, SessionPager<'a, M, S, T>> |
There was a problem hiding this comment.
It will be a breaking change in the API which will require a new major version release. Can you please add a new method (query_with_values or any other descriptive name) with a proposed signature and keep query in a previous shape?
There was a problem hiding this comment.
I read in this comment you are thinking about a new major release: #344 (comment). Is it still needed to remain backwards compatible? I think the new parameters should always be explicitly filled.
There was a problem hiding this comment.
Note: I made a new commit which adds a consistency level parameter and updated the 'Any' consistency documentation, since the documentation was wrong
examples/paged_query.rs
Outdated
| fn paged_with_values_list(session: &CurrentSession) { | ||
| let q = "SELECT * FROM test_ks.my_test_table where key in (?)"; | ||
| let mut pager = session.paged(2); | ||
| let mut query_pager = pager.query(q, Some(query_values!(vec![100, 101, 102, 103, 104, 105]))); |
There was a problem hiding this comment.
please try query values without vec!, just like query_values!(100, 101, ...)
I think it may solve the error you mentioned in a TODO comment
src/cluster/pager.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn query<Q>(&'a mut self, query: Q) -> QueryPager<'a, Q, SessionPager<'a, M, S, T>> | ||
| pub fn query<Q>(&'a mut self, query: Q, qv: Option<QueryValues>, consistency: Consistency) -> QueryPager<'a, Q, SessionPager<'a, M, S, T>> |
There was a problem hiding this comment.
maybe we can try to come up with some another method which would allow us to provide all values which make sense from https://github.com/AlexPikalov/cdrs/blob/master/src/query/query_params_builder.rs#L6 (perhaps every except the ones related to the paging itself)
Still I'd like to have query with as a minimalistic API
There was a problem hiding this comment.
Ok, in the newest commit this is done
There was a problem hiding this comment.
@AlexPikalov could you have another look? Thanks!
|
@AlexPikalov Please review so I can publish https://github.com/Jasperav/cdrs_orm |
src/cluster/pager.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn query<Q>(&'a mut self, query: Q) -> QueryPager<'a, Q, SessionPager<'a, M, S, T>> | ||
| where | ||
| Q: ToString, | ||
| { | ||
| self.query_with_pager_state(query, PagerState::new()) | ||
| self.query_with_param(query,QueryParamsBuilder::new() |
There was a problem hiding this comment.
Looks like it's not "cargo fmt"-ed.
| @@ -43,6 +44,7 @@ impl< | |||
| &'a mut self, | |||
| query: Q, | |||
| state: PagerState, | |||
| qp: QueryParams, | |||
There was a problem hiding this comment.
Seems it's going to be a breaking change which seems to be unnecessary.
Can you please introduce a new Pager method like query_with_pager_state_params. I know it_looks_a_bit_too_long but will help to keep it back compatible
There was a problem hiding this comment.
That is a good idea
| } | ||
|
|
||
| fn paged_with_values_list(session: &CurrentSession) { | ||
| let q = "SELECT * FROM test_ks.my_test_table where key in ?"; |
There was a problem hiding this comment.
I fixed the test! :)
|
FYI: This PR has been merged to https://github.com/krojew/cdrs-tokio |
@AlexPikalov can you help me on the TODO comment in the test? It fails for a vec when using an in parameter, but I can not figure it out why. This solves #336.