-
Notifications
You must be signed in to change notification settings - Fork 420
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
implement client set #11
Conversation
1e035d1
to
5397a54
Compare
7ac755b
to
3414827
Compare
3414827
to
928da11
Compare
still work to be done here but it's a start. right now this commit contains logic to handle `cargo run --bin client set key value` and nothing else
5397a54
to
0d29559
Compare
eda7343
to
2b6b19e
Compare
src/cmd/set.rs
Outdated
@@ -50,4 +67,12 @@ impl Set { | |||
debug!(?response); | |||
dst.write_frame(&response).await | |||
} | |||
|
|||
pub(crate) fn get_frame(self) -> Frame { |
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.
Idiomatically, this probably should be named into_frame
.
src/client.rs
Outdated
unimplemented!(); | ||
pub async fn set(&mut self, opts: Set) -> io::Result<()> { | ||
let frame = Command::Set(opts).to_frame()?; | ||
self.conn.write_frame(&frame).await |
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 guess we probably should validate the response?
eabf670
to
850462e
Compare
"unexpected response from server", | ||
)); | ||
if let Some(response) = response { | ||
match response { |
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.
We probably want to have a fn that tries to convert a frame into the various types. Then you can do response.try_into_string()
or something like that.
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.
interesting thought - seems probably worth a separate PR
src/client.rs
Outdated
Ok(self.conn.write_frame(&frame).await?) | ||
self.conn.write_frame(&frame).await?; | ||
let response = self.conn.read_frame().await?; | ||
let unknown_error = Box::new(Error::new( |
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.
We want to avoid unconditionally allocating here.
src/client.rs
Outdated
if response == "OK" { | ||
Ok(()) | ||
} else { | ||
Err(unknown_error) |
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.
Err(unknown_error) | |
Err("unexpected response from server".into()) |
This should work. The error message could be set to a const too to avoid duplicating it.
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.
👍 TIL!
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.
Gonna merge as is 👍 can polish in further PRs.
Continuation of #11. Refines the client structure and implements GET. `clap` is decoupled from the lib code. This is done to avoid any CLI parsing concerns to leak into the lib. The main motivation for this is to allow the reader to focus on Tokio concerns and not CLI parsing concerns.
Continuation of #11. Refines the client structure and implements GET. `clap` is decoupled from the lib code. This is done to avoid any CLI parsing concerns to leak into the lib. The main motivation for this is to allow the reader to focus on Tokio concerns and not CLI parsing concerns.
still work to be done here but it's a start. right now this commit
contains logic to handle
cargo run --bin client set key value
andnothing else
TODO:
i have an open question about usage and specifying EX/PX here, and still need to send the proper frames when our duration argument is sent