Re: [pbs-devel] [PATCH proxmox v2 1/4] sendmail: add sendmail crate

[Lukas Wagner <l.wagner@proxmox.com>]

Thanks for the quick v2, Shannon!

Two comment inline


On  2024-12-02 13:58, Shannon Sterz wrote:
> add the `proxmox-sendmail` crate that makes it easier to send mails via
> the `sendmail` utility. features include:
> 
> - multipart/alternative support for html+plain text mails
> - multipart/mixed support for mails with attachments
> - automatic nesting of multipart/alternative and multipart/mixed parts
> - masks multiple receivers by default, can be disabled
> - encoding Subject, To, From, and attachment file names correctly
> - adding an `Auto-Submitted` header to avoid triggering automated mails
> 
> also includes several tests to ensure that mails are formatted
> correctly.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> 
> changes since v1 (thanks @ Lukas Wagner <l.wagner@proxmox.com>):
>     - make it possible to disable receiver redaction
>     - re-structure the mal formatting code; mainly split it into
>       multiple functions (`format_header`, `format_body`,
>       `format_attachment` etc.)
>     - fix multiple typos

You are still missing Debian packaging :)

✂ snip ✂

> +
> +    fn format_header(
> +        &self,
> +        now: i64,
> +        file_boundary: &str,
> +        html_boundary: &str,
> +    ) -> Result<String, Error> {
> +        let mut header = String::new();
> +
> +        let encoded_to = if self.to.len() > 1 && self.mask_participants {
> +            // if the receivers are masked, we know that they don't need to be encoded
> +            false
> +        } else {
> +            // check if there is a recipient that needs encoding
> +            self.to.iter().any(|r| !r.is_ascii())
> +        };
> +
> +        if !self.attachments.is_empty() {
> +            header.push_str("Content-Type: multipart/mixed;\n");
> +            header.push_str(&format!("\tboundary=\"{file_boundary}\"\n"));

Sorry I missed this in v1, but I think this could be a bit nicer as 

    use std::fmt::Write;

    writeln!(&mut header, "\tboundary=\"{file_boundary}\")?;

This is a bit shorter, easier to read and should be a tiny bit faster as well
(not that it makes an actual noticeable difference here).

Of course this also applies to the other places where you append to the header/body.


-- 
- Lukas



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel