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

["Shannon Sterz" <s.sterz@proxmox.com>]

On Mon Dec 2, 2024 at 2:30 PM CET, Lukas Wagner wrote:
> 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 ✂

ah yes, sorry fixed that up in a v3 just now

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

alright, switch over instances of `push_str(&format!())` over to
`write!` and `writeln!`


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