jonhoo / rust-imap

IMAP client library for Rust
Apache License 2.0
477 stars 80 forks source link

Add support for RFC 5819: LIST-STATUS extension #249

Closed bitfehler closed 1 year ago

bitfehler commented 1 year ago

See https://tools.ietf.org/html/rfc5819

Hi there. I would love to add support for RFC 5819. It is extremely valuable for clients to perform a full mailbox sync with as little requests as possible. This patch is not yet well-documented, but I wanted to get some feedback early on, specifically on the following:

And of course whatever else catches your eye :slightly_smiling_face:


This change is Reviewable

bitfehler commented 1 year ago

Oh, and I can of course add tests once we agree on the rest :wink:

codecov[bot] commented 1 year ago

Codecov Report

Merging #249 (247e36b) into main (64e7b4a) will increase coverage by 0.2%. The diff coverage is 88.0%.

Additional details and impacted files | [Impacted Files](https://codecov.io/gh/jonhoo/rust-imap/pull/249?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset) | Coverage Δ | | |---|---|---| | [src/client.rs](https://codecov.io/gh/jonhoo/rust-imap/pull/249?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL2NsaWVudC5ycw==) | `93.1% <ø> (ø)` | | | [src/extensions/list\_status.rs](https://codecov.io/gh/jonhoo/rust-imap/pull/249?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL2V4dGVuc2lvbnMvbGlzdF9zdGF0dXMucnM=) | `88.0% <88.0%> (ø)` | | | [src/types/fetch.rs](https://codecov.io/gh/jonhoo/rust-imap/pull/249?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3R5cGVzL2ZldGNoLnJz) | `65.1% <0.0%> (+4.6%)` | :arrow_up: |
bitfehler commented 1 year ago

@jonhoo gentle ping :innocent: would be great to know if you have any opinions?

jonhoo commented 1 year ago

This looks entirely reasonable to me (subject to tests being added)! I'm a little surprised to see Option<Mailbox>, but I guess the server can just choose to not include any mailbox info in there?

I forget why Mailbox has some non-Option fields. I thought I made it accurately mirror what the spec said were required fields, but may have mis-parsed? I think we should leave it as-is for this PR though!

bitfehler commented 1 year ago

I'm a little surprised to see Option<Mailbox>, but I guess the server can just choose to not include any mailbox info in there?

The RFC is a bit weird. It says the server MUST include that information, but goes on to explicitly specify that in certain error situations the server may omit it. I've never seen that happen, but you never know...

I updated the documentation a bit and added some tests, including an integration test. I think the coverage should be quite good, but for some reason the coverage runner seems to have problems uploading the report? :thinking:

Note: I also added a commit that changes the annotation for the qresync integration test from #[ignore] to #[cfg(feature = "test-full-imap")]. I wrote that test, and it works fine against e.g. Cyrus. But I think that feature did not exist back then? Anyways, seemed like the right thing to do while I was touching that file.

jonhoo commented 1 year ago

Not sure why the CI step is being sad. I wonder if it just needs a re-run..

bitfehler commented 1 year ago

Sorry for the double update, just now saw that you fixed the coverage upload on main.

jonhoo commented 1 year ago

Hmm, it looks like now none of the CI steps ran :thinking: Can you try rebasing onto main and force-pushing?

jonhoo commented 1 year ago

Released in alpha.10 :tada: