client: extend APPEND to support optional flags

[cyphar]

According to RFC3501, the APPEND command supports optional flags for
the datetime of the message receipt, and a set of flags to be applied to
the message. Implement this in imap-rust, using the same formatting as
the Python imaplib equivalent code.

Fixes #60
Signed-off-by: Aleksa Sarai [email protected]

[coveralls]

Coverage increased (+17.9%) to 79.892% when pulling 5a3cc33 on cyphar:append-extra-options into 4332d09 on mattnenterprise:master.

[coveralls]

Coverage increased (+17.9%) to 79.892% when pulling 5a3cc33 on cyphar:append-extra-options into 4332d09 on mattnenterprise:master.

[coveralls]

Coverage increased (+17.9%) to 79.892% when pulling 6021160 on cyphar:append-extra-options into 4332d09 on mattnenterprise:master.

[cyphar]

I've tested this on Mailbox.org and GMail, and I'm under the impression it works as-expected.

[jonhoo]

Thank you! Three thoughts:

• I'm a little hesitant to introduce a new dependency (chrono) for seemingly fairly little gain. Is there a particular reason you chose that instead of, say, SystemTime?
• This changes the signature of append in a backwards-incompatible way, and adds a number of extra arguments. I don't have a good idea of how often those will be None. If it's often, I wonder if we might want to instead add a different method (perhaps append_with_flags?). Otherwise, the backwards-incompatability may be worth it given that we're also (likely) merging the large change in Add structured results for all values using imap-proto #58. What are your thoughts on this?
• I'd like to see a rudimentary test for this that it generates the right requests when called with/without flags/times.

[cyphar]

• I'm a little hesitant to introduce a new dependency (chrono) for seemingly fairly little gain. Is there a particular reason you chose that instead of, say, SystemTime?

Unless I'm mistaken, don't we already indirectly depend on chrono (it's very widely used)? If not, then I guess we could use something else, but from my understanding SystemTime doesn't let you format it with strftime-style format strings, which is necessary for IMAP. Another option would be to take a string and punt on having semantic types in the API (which is what has been done for other methods) but I'm not sure that's a good idea either.

• This changes the signature of append in a backwards-incompatible way, and adds a number of extra arguments. I don't have a good idea of how often those will be None. If it's often, I wonder if we might want to instead add a different method (perhaps append_with_flags?).

This is fair enough (though this library is pre-1.0). I will add another method rather than changing the existing one. As for how common this usage is, I would expect it depends on how you're using IMAP. But from my reading of RFC3501 the only way of setting the "datetime" of a message in IMAP is through APPEND, which makes having these flags necessary for some usecases (like mine).

• I'd like to see a rudimentary test for this that it generates the right requests when called with/without flags/times.

Will do.

[jonhoo]

Unless I'm mistaken, don't we already indirectly depend on chrono (it's very widely used)?

You're right that it's common, though I actually don't think we currently do! If I'm wrong, I still think there's an advantage to interacting with standard types first unless other types present a significant advantage though.

from my understanding SystemTime doesn't let you format it with strftime-style format strings, which is necessary for IMAP.

Yeah, I just took another gander at the docs, and looks like there is simply no way to print them at all. So I guess ignore what I said above, and consider chrono a necessary dependency :)

Another option would be to take a string and punt on having semantic types in the API (which is what has been done for other methods) but I'm not sure that's a good idea either.

Nope nope nope, let's not do that. This is part of why I wrote #58; I want to get away from non-semantic types.

This is fair enough (though this library is pre-1.0).

While it's true the crate is pre-1.0, I think it likely should be. We have some users, and breaking things isn't great. I think it's only not 1.0 because in the past 0.X wasn't considered materially different from 1.X. This is obviously changing. Once #58 lands, I think we should seriously consider tagging 1.0.

I will add another method [...]

Thanks! I know it's a little annoying, though I'd rather have more methods that are more explicit and have fewer arguments.

[cyphar]

This is part of why I wrote #58; I want to get away from non-semantic types.

As an aside, this is something I'm looking forward to, because I'm currently parsing IMAP4 return messages using regular expressions. As someone coming from Python (which has imaplib and email) this is something that is sorely missing from all the IMAP4 crates I've looked at.

[jonhoo]

https://github.com/djc/tokio-imap may also have what you want as an alternative.

[coveralls]

Coverage increased (+17.9%) to 79.892% when pulling e15d8b8 on cyphar:append-extra-options into 4332d09 on mattnenterprise:master.

[jonhoo]

This completely escaped my mind, so sorry about that! I think you'll need to rebase onto master, but shouldn't be too bad (I don't think append changed all that much). I think I would like to ultimately see a builder-like pattern for something like append that takes this many arguments, but this is a fine starting point.

I would love to see a test of this method to see that it generates the expected IMAP commands. In particular, with different combinations of arguments (_, Some, Some, _, _, Some, None, _, etc.)

[jonhoo]

I think we also want to align this with #99 somehow.

[cyphar]

I haven't touched this in a very long time... Feel free to carry or close it.

Personally this was one of my first projects in Rust, and I was quite disappointed that I couldn't find a Rust library that handled IMAP correctly (Python has it in stdlib, the Go one is pretty solid, and so on) -- to me it indicated that the language's ecosystem isn't ready yet (I've written other system tools in Rust, but I ended up just wiring up most of the syscalls myself -- so I was just using the core language).

Don't take this as me saying I'm unhappy with you specifically or anything, it's just an expected outcome when you have a not-yet-mature language ecosystem.

I do really hope that the Rust community will come up with a core set of third-party libraries that are generally well-maintained and do their job completely and correctly, because I still am excited about the language but the way third-party libraries work (especially with the ease of creating crates -- something I really disliked about NPM and now I dislike about crates.io) makes me quite hesitant about it all.

[jonhoo]

You're certainly right that Rust's ecosystem is still young, but I think things have improved since you first looked at this. I'm not quite sure what your specific concern is with "the way third-party libraries work" though? I've found cargo and crates.io to work extremely well compared to basically every other language out there. But YMMV I suppose :)

For imap specifically, I finally now have publish access to this repository (we had some issues in #69), and just released 0.9 which has some serious improvements. I think adding these extra arguments to append is a blocker for imap 1.0, but I also don't think we want to add it quite this way. You'd just end up with tons of arguments, many of which the user usually won't care about. Something like a builder pattern (like proposed in #99) would likely work better.

I'll leave this open for the time being, as I believe it is an important thing to add, and this is a good starting point.

[jonhoo]

I've had to move the repository under my own control so that I can set up CI and such correctly following the resolution to #69, so I will actually close this. Feel free to open it as a new PR against https://github.com/jonhoo/rust-imap if you have the time, but no worries if you don't! If you do, including a link back to this for later reference is probably also a good idea :)