Remove non-SMTP transports and connection setup code #57
Conversation
link2xt
force-pushed
the
version2.0
branch
4 times, most recently
from
f03cc78 to
330f077
Compare
| let mut codec = ClientCodec::new(); | ||
| let mut buf: Vec<u8> = vec![]; | ||
|
|
||
| assert!(codec.encode(b"test\r\n", &mut buf).await.is_ok()); |
There was a problem hiding this comment.
Maybe use Result/? to be less verbose?
There was a problem hiding this comment.
This test was only moved around, it's not new code, so I think it makes sense to leave it as is.
|
|
||
| /// Sends an AUTH command with the given mechanism, and handles challenge if needed | ||
| pub async fn auth(&mut self, mechanism: Mechanism, credentials: &Credentials) -> SmtpResult { | ||
| // TODO |
There was a problem hiding this comment.
This is not new code, it is moved from src/smtp/client/inner.rs as is.
| .command(AuthCommand::new(mechanism, credentials.clone(), None)?) | ||
| .await?; | ||
|
|
||
| while challenges > 0 && response.has_code(334) { |
There was a problem hiding this comment.
Define a constant with a comment?
There was a problem hiding this comment.
Same, moved code. Would be nice to cleanup, but I want to avoid changing it in this PR.
| } | ||
|
|
||
| if challenges == 0 { | ||
| Err(Error::ResponseParsing("Unexpected number of challenges")) |
There was a problem hiding this comment.
"Number of challenges > N" would be more informative
There was a problem hiding this comment.
Again, the code was only moved
| sent_commands += 1; | ||
|
|
||
| for _ in 0..sent_commands { | ||
| self.stream.read_response().await?; |
There was a problem hiding this comment.
Maybe read responses in parallel with sending commands? Can't RX overflow and server stop reading from the socket in the current approach?
There was a problem hiding this comment.
I opened an issue at #58 so we don't forget,
but it's out of scope for this PR.
| loop { | ||
| let read = reader.read_line(&mut buffer).await?; | ||
| if read == 0 { | ||
| break; |
There was a problem hiding this comment.
Why not return right from here? And does it need a special handling? Won't buffer be empty and parse_response() fail as needed?
|
|
||
| impl<S: Read + Write + Unpin> SmtpTransport<S> { | ||
| /// Creates a new SMTP transport and connects. | ||
| pub async fn new(builder: SmtpClient, stream: S) -> Result<Self, Error> { |
There was a problem hiding this comment.
It took me a while to get why it's called builder, maybe call it smtp_client for the sake of least astonishment?
| } | ||
|
|
||
| if challenges == 0 { | ||
| Err(Error::ResponseParsing("Unexpected number of challenges")) |
There was a problem hiding this comment.
Again, the code was only moved
src/types.rs
Outdated
| pub fn new(envelope: Envelope, message: impl AsRef<[u8]>) -> SendableEmail { | ||
| SendableEmail { | ||
| envelope, | ||
| message_id: message_id.as_ref().into(), | ||
| message: Message::Bytes(Cursor::new(message.as_ref().to_vec())), |
There was a problem hiding this comment.
Why make message: impl AsRef<[u8]> and then always call message.as_ref().to_vec()? Wouldn't it make sense to just make message: Vec<[u8]>? Then the caller has to sometimes call to_vec() themselves, but that seems OK.
After I wrote my comment, I noticed that you didn't introduce the AsRef, so we can just leave it as is for now. Then again, a major version bump is the only chance to do this change. Anyway, do as you like :)
link2xt
changed the title
Now the user is responsible for providing a stream with TLS, read and write timeouts, buffering, SOCKS5 etc. This makes it possible to have proper read and write timeouts instead of per-command timeouts, use alternative TLS implementations such as Rustls and use SMTP over other proxies, such as HTTP CONNECT, without modifying the library.
Remove connection setup and all the "transports" except SMTP.
Now the user is responsible for providing a stream with TLS, read and write timeouts, buffering, SOCKS5 etc.
This makes it possible to have proper read and write timeouts instead of per-command timeouts,
use alternative TLS implementations such as Rustls and use SMTP over other proxies, such as HTTP CONNECT, without modifying the library.