-
Notifications
You must be signed in to change notification settings - Fork 233
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
Table API query/get all rewrite #84
Conversation
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.
Looking good!
Some(States::Init), | ||
move |state: Option<States>| async move { | ||
log::debug!("state == {:?}", state); | ||
let response = match state { |
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.
nit: maybe match state?
would make the code a little less verbose.
Co-authored-by: Ryan Levick <rylev@users.noreply.github.com>
Co-authored-by: Ryan Levick <rylev@users.noreply.github.com>
Co-authored-by: Ryan Levick <rylev@users.noreply.github.com>
…r-rust into issue/83/table_paging
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 fulfils #83 nicely but I think it may have a couple typos and the dummy URL leaking out of the public interface of ContinuationToken
is a bit strange.
Co-authored-by: Alexei Barnes <ab@alexeibarnes.com>
Co-authored-by: Alexei Barnes <ab@alexeibarnes.com>
pub continuation_token: Option<ContinuationToken>, | ||
} | ||
|
||
impl<T> std::convert::TryFrom<(url::Url, &HeaderMap, &body::Bytes)> for QueryResult<T> |
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 always find conversion methods from tuples to be very confusing unless it's an extremely obvious one like (f32, f32)
to Point
. Is there a better way to model this?
} | ||
} | ||
|
||
impl std::convert::From<(Url, ResponseFuture)> for PerformRequestResponse { |
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.
Not sure I like this. I think I'd prefer a PerformRequestResponse::new
function. Tuples are best for data that is extremely obviously related to one another, which is not the case here. The new function will be a bit easier to understand.
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.
Looks great! I have some nits, but feel free to merge and we can address those later.
A proposal to address #83.