WIP: split Client into UnauthenticatedClient and Client
#84
Conversation
src/client.rs
Outdated
| match self.inner.run_command(&format!("AUTHENTICATE {}", auth_type)) { | ||
| Ok(_) => self.do_auth_handshake(authenticator), | ||
| Err(e) => Err((e, self)), | ||
| } |
There was a problem hiding this comment.
Given the repetition of this, you may want to add a simple macro for it.
|
I think this looks like a good approach! I'd suggest the following for naming/method placement:
We could go even further and rename The nicest way to work around all the |
as suggested in PR mattnenterprise#84 comments
is there some way to include the code directly from examples/basic.rs?
src/client.rs
Outdated
| @@ -327,13 +359,14 @@ impl<T: Read + Write> UnauthenticatedClient<T> { | |||
| mut self, | |||
| username: &str, | |||
| password: &str | |||
| ) -> ::std::result::Result<Client<T>, (Error, UnauthenticatedClient<T>)> { | |||
| ) -> ::std::result::Result<Session<T>, (Error, Client<T>)> { | |||
There was a problem hiding this comment.
Probably make Client<T> here just Self?
|
@jonhoo changed the naming of the various structs, implemented your suggestions on depending on your preference i can squash some of the commits into one to avoid e.g. one iteration back and forth of adding |
There was a problem hiding this comment.
I'm happy for you to leave the commits separate -- history is nice. Other than that, this change looks quite good! It requires a major version bump, but sadly I can't make a release due to #69 :/
The remaining things as far as I can tell are to add documentation for Session and Client. It'd also be nice to add a rust,no_run doctest example to connect and secure_connect (even though I know the original methods didn't have one). I also have a somewhat silly request that you can ignore if you don't think it's worth it: could you move the
impl <T: Read + Write> Connection<T> {block where you have fn run_command_and_check_ok and such down to where those methods were previously defined in the file? It'll make the diff much smaller and easier to review. I think all that should be needed is to cut-paste that entire block down around line 713.
Thanks for all the great work!
|
Re 515d574: No, sadly not. There's the |
src/client.rs
Outdated
| username: &str, | ||
| password: &str | ||
| ) -> ::std::result::Result<Session<T>, (Error, Client<T>)> { | ||
| // note that we need the explicit match blocks here for two reasons: |
There was a problem hiding this comment.
Should this comment now go away? Or perhaps be included in the macro?
There was a problem hiding this comment.
ah, yes.. next commit (also including the comments from above) soon-ish :-)
as discussed/suggested at mattnenterprise#84 (review)
|
docs (including moving the comment to the macro definition) and some examples added.
i've been on both sides of very similar requests before, so happily included this one as well :-) |
as discussed/suggested at mattnenterprise#84 (review)
|
Great, almost there! I think the comment still needs to be removed from I'll merge once that's fixed, though @mattnenterprise will need to do a (major) release. Hopefully he'll also fix #69 when he's at it. |
ah, right. pushed. |
|
Thank you! |
a first attempt at the split between the unauthenticated (but connected) and the logged in state on the type level, as discussed in the comments of issue #80 . the only way to get a (logged in)
Clientinstance is through successful calls to thelogin/authenticatemethods. implementation-wise there's now a third type,InnerClient, which contains all lower-level functionality used by bothUnauthenticatedClientandClient(to avoid duplicating the code or having weird cross-references between theClientandUnauthenticatedClienttypes.as i'm mostly looking for feedback on the general direction of how i implemented the idea, there's not enough docs and i haven't ran
rustfmtorcargo testyet etc. i'm especially hoping for feedback onconnect/secure_connectto stay in theClienttype (but return anUnauthenticatedClient), so people can continue to writeClient::connect(..), or is it better to make the API break explicit here, as you have to deal with the return value of connect (theUnauthenticatedClient) differently now (i.e. don't provide a false sense of API continuity)?InnerClientto theClienttype, so we could reverse the changes to the (non-involved)IdleHandle. for now i only added wrappers for those that were markedpub.i'm always happy to change things and re-submit. also i wasn't sure whether
UnauthenticatedClient/Clientor e.g.Client/AuthenticatedClientis the better naming; if people have thoughts on that i'm happy to hear those, too (in this first attempt i opted for the former, as this keeps more of the interface on the existingClienttype).