search

stalwartlabs / mail-send

E-mail delivery library for Rust with DKIM support
https://docs.rs/mail-send/
Apache License 2.0
202 stars 21 forks source link

Remove skip-ehlo feature, replace with config option #23

Closed hoxxep closed 8 months ago

hoxxep commented 10 months ago

Hi, I love the work on Stalwart mail server!

Depending on the mail-send crate across a larger codebase sometimes requires skipping EHLO and sometimes doesn't. Using compile-time flags results in accidental behaviour where all usage either skips or doesn't skip, or care is needed to compile and use two separate dependencies, with some other headaches potentially requiring multiple mail-builder deps.

This PR removes the skip-ehlo compile-time feature in favour of a config option in the SmtpClientBuilder for usability. If you're happy to include this PR, I can make a matching PR to fix usages in mail-server and any other stalwart repos that use the skip-ehlo feature.

Let me know if there's anything else I can do to make this an easy change.

Matching PR: https://github.com/stalwartlabs/mail-server/pull/121

Cheers, Liam

hoxxep commented 8 months ago

A better reason for why a config may be preferable is because rust features are meant to be additive. [1]

If crates A and B both depend on crate D, A might require feature 1, while B might require feature 2, and rust assumes that in the final build it can compile D once with features 1+2 enabled.

In this case, the skip-ehlo feature is subtracting functionality, causing me to carefully depend on two builds of mail-send, or always manually run EHLO. Hopefully this PR can be considered to remove the need for the feature gate altogether. Thank you!

[1] https://doc.rust-lang.org/cargo/reference/features.html#:~:text=A%20consequence%20of%20this%20is,enable%20any%20combination%20of%20features.

mdecimus commented 8 months ago

Hi @hoxxep thanks for the PR! I've been busy working on Stalwart Mail Server and couldn't find time to review it as it affects multiple parts of the project. I will probably just remove this feature from mail-send and implement the functionality directly in Stalwart Mail Server.

mdecimus commented 8 months ago

Closing this PR as this skip-helo feature has been removed.