piknik icon indicating copy to clipboard operation
piknik copied to clipboard

Some public API suggestions

Open djc opened this issue 3 years ago • 3 comments

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!

djc avatar Nov 11 '21 16:11 djc

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.

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 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).

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 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).

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.

paolobarbolini avatar Nov 15 '21 14:11 paolobarbolini

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.

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.

djc avatar Nov 15 '21 14:11 djc

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).

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?

What do you guys think about the secstr crate ?

DK26 avatar Apr 03 '22 18:04 DK26