Closed lu-fennell closed 2 years ago
Ah, yes, I agree, that would be helpful! Would definitely love a PR :+1:
Cool, I'm on it.
Btw, what do you think about defining commands like this?:
let command = Command::new(
"LOGIN",
[
Arg::quoted("username", username.as_ref()),
Arg::quoted("password", password.as_ref()),
],
);
let command_string = ok_or_unauth_client_err!(command.format(), self);
ok_or_unauth_client_err!(self.run_command_and_check_ok(&command_string), self);
where format
loops through the arguments, performs the appropriate check and builds up a String
if successful.
It should also be possible to pass a Command
directly to the run_command_*
functions, saving the first invocation of ok_or_unauth_client_err!
:
ok_or_unauth_client_err!(self.run_command_and_check_ok(command), self);
Do you find something like this acceptable or would you prefer me to take a more "conservative" approach? E.g.:
let synopsis = "LOGIN username password";
let u =
ok_or_unauth_client_err!(validate_str(synopsis, "username", username.as_ref()), self);
let p =
ok_or_unauth_client_err!(validate_str(synopsis, "password", password.as_ref()), self);
ok_or_unauth_client_err!(
self.run_command_and_check_ok(&format!("LOGIN {} {}", u, p)),
self
);
I opened a PR with the straightforward approach, for now.
Btw, what do you think about defining commands like this?: [...]
I see that imap-proto
already provides command builders. They also do validation of quoted strings. Any particular reason why imap
is not using them, or is not providing an API that accepts imap-proto
commands?
We had a discussion about whether we should have command builders a while ago, and decided that it only make sense for some commands, and not all. For example, for select
, it's not clear that a builder really makes sense. I'm also hesitant to increase our re-exporting of imap-proto
types because it makes us more vulnerable to breaking changes in imap-proto
.
Also cc @mordak
I don't think it makes sense to re-export the imap_proto::builders::command
bits. It would be fine to use them internally if they make sense, but presently the failure mode for an invalid character in, say, a password, is a panic and a message that you cannot include CR or LF in a quoted string. This isn't much more friendly than what we do now, and the changes in this PR make this crate's error message way more friendly.
I also am not sure if there is a lot of utility in adding builders for a bunch of commands when it is often simpler to just call, say, client.login(username, password)
and get your Result
immediately (and if there is an Err
, it provides a useful description).
Hi. I accidentally added a newline to a password and my program errored-out with the very undescriptive message:
resp
I think a message along the lines of
resp
would be more helpful.
I could propose a corresponding pull request, if you are interested.