refactor: squash append command

[mblarsen]

This is an attempt to simplify append by squashing the three variants into to one command + an options struct AppendOptions.

This PR also provides a command builder AppendCmd to simplify construction of the options struct as suggested here and onwards:

session.append(
  mbox,
  e.message_to_string().unwrap(),
  AppendCmd::new()
     .flag(Flag::Seen)
     .internal_date(&date)
);

• tests
• update docs

---

Thoughts not related to what is implemented:

• If AppendCmd builds options maybe it should be called AppendOptionsBuilder (very long)

• and if so what AppendCmd look like:

struct AppendCmd {
  mbox: ..,
  content: ..,
  // impl
  pub fn run(&self) -> ...;
  pub fn with_options(&self) -> AppendOptionsBuilder {
     AppendOptionsBuilder {
         cmd: &self,
         flags: ...
         internal_date: ...
     }
     // impl
     fn run(&self) {
         self.cmd.run()
     }
  } 
}

session.append(mbox, e.message_to_string().unwrap()) |> AppendCmd
  .with_options() |> AppendOptionsBuilder
  .flag(Flag::Seen) |> AppendOptionsBuilder
  .internal_date(date) |> AppendOptionsBuilder
  .run()

Or maybe just combine them into to one. Compared to the existing api aside from the conveinince of builders there is an extra call to run()

---

This change is 

[jonhoo]

Oooh, yes, I like the idea of append returning a builder instead! Just make sure you annotate it with #[must_use]. And that I think will also take care of your lifetime issue if you instead have just one type.

[mblarsen]

So I refactored it to only have AppendCmd doing away with AppendOptions. I'm facing a new issue with lifetimes. In order to be able to invoke run() at the end we'll need a reference to session. I think this is where the lifetime issue arises:

Here is an example usage from the tests:

The original append is now reduced to this:

    pub fn append<'a, S: AsRef<str>, B: AsRef<[u8]>>(
        &mut self,
        mailbox: S,
        content: B,
    ) -> AppendCmd<'a, T> {
        AppendCmd {
            session: &self,
            content: content.as_ref(),
            mailbox: mailbox.as_ref(),
            flags: Vec::new(),
            date: None,
        }
    }

Again I tried a bunch of variations, e.g. adding a separate lifetime to the session ref, but couldn't quite grok it.

This is the issue:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
    --> src/client.rs:1165:9
     |
1165 |         AppendCmd {
     |         ^^^^^^^^^
     |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the method body at 1160:5...
    --> src/client.rs:1160:5
     |
1160 | /     pub fn append<'a, S: AsRef<str>, B: AsRef<[u8]>>(
1161 | |         &mut self,
1162 | |         mailbox: S,
1163 | |         content: B,
1164 | |     ) -> AppendCmd<'a, T> {
     | |_________________________^
note: ...so that reference does not outlive borrowed content
    --> src/client.rs:1166:22
     |
1166 |             session: &self,
     |                      ^^^^^
note: but, the lifetime must be valid for the lifetime `'a` as defined on the method body at 1160:19...
    --> src/client.rs:1160:19
     |
1160 |     pub fn append<'a, S: AsRef<str>, B: AsRef<[u8]>>(
     |                   ^^
note: ...so that the expression is assignable
    --> src/client.rs:1165:9
     |
1165 | /         AppendCmd {
1166 | |             session: &self,
1167 | |             content: content.as_ref(),
1168 | |             mailbox: mailbox.as_ref(),
1169 | |             flags: Vec::new(),
1170 | |             date: None,
1171 | |         }
     | |_________^
     = note: expected `AppendCmd<'a, _>`
                found `AppendCmd<'_, _>`

What I'm failing to understand is what the "anonymous lifetime" that is being referred to? As far as I can tell, they are explicit.

[jonhoo]

Hmm... I think you'll need:

    pub fn append<'a, S: AsRef<str>, B: AsRef<[u8]>>(
        &'a mut self,
        mailbox: S + 'a,
        content: B + 'a,
    ) -> AppendCmd<'a, T> {
        AppendCmd {
            session: &self,
            content: content.as_ref(),
            mailbox: mailbox.as_ref(),
            flags: Vec::new(),
            date: None,
        }
    }

[mblarsen]

Hmm... I think you'll need:

Almost, but not quite. But it showed me in the right direction, so thank you for that. You'd better do a final review.

I've updated the tests + docs (moved flag + date docs to the AppendCmd)

Also added a flags() in addition to flag() in case you have your flags already come as a slice of flags.

I'm not sure how to test must_use. I tried removing run() from the tests, but the compiler didn't complain.

[mblarsen]

I don't know if I should worry about AppendCmd::flags is a vector. Would IMAP complain if I passed two Seen flags?

[jonhoo]

This is starting to look really good!

I don't know if I should worry about AppendCmd::flags is a vector. Would IMAP complain if I passed two Seen flags?

I think it's probably okay for us to ignore that here, and leave it to the caller to just not do that.

[mblarsen]

Thanks for the feedback. I've refactored accordingly + some improvements to docs.

The tests are failing to compile right now because of .flags() the function that takes multiple flags at once.

The issue has to do with the impl IntoIterator<...> changes. The error given seems a bit inverted to me:

When called like this:

    let flags: &[Flag] = &[Flag::Seen, Flag::Flagged];
    c.append(mbox, e.message_to_string().unwrap().as_bytes())
        .flags(flags)
        .finish()
        .unwrap();

What makes it think that a &Flag<'_> is expected? Is it because values in a vector (what we extend) are all references?

Aside: this PR is for the version 3 milestone I guess.

[jonhoo]

Ah, yes, it's because flags takes an iterator of owned Flags, not of references to Flag. Try calling it like:

    let flags = vec![Flag::Seen, Flag::Flagged];
    c.append(mbox, e.message_to_string().unwrap().as_bytes())
        .flags(flags)
        .finish()
        .unwrap();

or

    let flags = &[Flag::Seen, Flag::Flagged];
    c.append(mbox, e.message_to_string().unwrap().as_bytes())
        .flags(flags.into_iter().cloned())
        .finish()
        .unwrap();

[mblarsen]

I think we are there now.

I'm still not sure why it expected a reference to Flag. It seems the other way around, that it got a reference to Flag and expected an owned Flag. (I'm referencing the note in the error message)

Are &[T] always references?

[jonhoo]

Yeah, the error message did seem a little unhelpful, I agree. 🤷
But yes, iterators you get from &[T] always produce &T, so that's likely where the error stems from :)