piknik
piknik copied to clipboard
Some public API suggestions
Is your feature request related to a problem? Please describe. Getting started with lettre, these are the imports I need for a minimal setup:
use lettre::message::Message;
use lettre::transport::smtp::authentication::Credentials;
use lettre::transport::smtp::AsyncSmtpTransport;
use lettre::{AsyncTransport, Tokio1Executor};
It seems like it would be better if transport
was not an explicit module in the public API, since lettre::smtp
would probably be obvious enough.
Async
prefixes seem a bit dated, IMO it would be better to have sync
and async
modules (or, keep async
code -- which IMO is more common for networking code -- in the crate root and isolate the sync APIs in a sync
module).
Having to import and initialize Credentials
doesn't seem particularly useful, might be better if AsyncSmtpTransportBuilder::credentials()
took username and password arguments directly (for bonus points, take something like impl Into<Cow<'static, str>>
so I don't have to convert &'static str
manually and you don't have to allocate for static strings).
Just some ideas for improvements. Thanks for all the work on this crate, @paolobarbolini your work across the ecosystem is very impressive!
It seems like it would be better if
transport
was not an explicit module in the public API, sincelettre::smtp
would probably be obvious enough.
This one is more for Alexis since the change was done either before I joined the project or very early on. What I can say is that although having a transport
module makes for longer import lines it also gives a stronger separation between the email transport and email builder sections of the crate. It's also a nice place to put documentation generic to the various email transports https://docs.rs/lettre/0.10.0-rc.4/lettre/transport/index.html
Async
prefixes seem a bit dated, IMO it would be better to havesync
andasync
modules (or, keepasync
code -- which IMO is more common for networking code -- in the crate root and isolate the sync APIs in async
module).
I'm a bit on the fence on this. On one hand yes networking code tends to be used in async contexts, and many crates dropped the async prefix, on the other it's nice not to have to worry about multiple structs from the same crate having the same name.
Having to import and initialize
Credentials
doesn't seem particularly useful, might be better ifAsyncSmtpTransportBuilder::credentials()
took username and password arguments directly (for bonus points, take something likeimpl Into<Cow<'static, str>>
so I don't have to convert&'static str
manually and you don't have to allocate for static strings).
One advantage of Credentials
is that it's Debug implementation doesn't leak the SMTP server's credentials. It could also, although we currently don't do this, zeroize the underlying memory on Drop
. Other than that I think we could do without it.
I'm not sure about the &'static str
use-case. The only thing that comes to mind is hard coding credentials, which I don't see as a good default that lettre should support. I've had other projects though request the same thing in the past, so maybe there's a use-case that I'm missing?
Just some ideas for improvements. Thanks for all the work on this crate, @paolobarbolini your work across the ecosystem is very impressive!
Thanks for the suggestions. It's good to see you try lettre :smiley:. I'm not against any of these points, I'd just like to know more about it. For things like removing the transport
module I also don't know/remember the full context of why the change was done (lettre 0.9 does it the way you describe https://docs.rs/lettre/0.9), so I would want to look more into it before making any decision.
One advantage of
Credentials
is that it'sDebug
implementation doesn't leak the SMTP server's credentials. It could also, although we currently don't do this, zeroize the underlying memory onDrop
. Other than that I think we could do without it.
You could still wrap the credentials in Credentials
internally, as it is there just seems to be fairly little point in requiring the caller to wrap the credentials in Credentials
.
Having to import and initialize
Credentials
doesn't seem particularly useful, might be better ifAsyncSmtpTransportBuilder::credentials()
took username and password arguments directly (for bonus points, take something likeimpl Into<Cow<'static, str>>
so I don't have to convert&'static str
manually and you don't have to allocate for static strings).One advantage of
Credentials
is that it's Debug implementation doesn't leak the SMTP server's credentials. It could also, although we currently don't do this, zeroize the underlying memory onDrop
. Other than that I think we could do without it. I'm not sure about the&'static str
use-case. The only thing that comes to mind is hard coding credentials, which I don't see as a good default that lettre should support. I've had other projects though request the same thing in the past, so maybe there's a use-case that I'm missing?
What do you guys think about the secstr
crate ?