duesee / imap-codec

Rock-solid and complete codec for IMAP
Apache License 2.0
35 stars 13 forks source link

CONDSTORE #439

Open superboum opened 4 months ago

superboum commented 4 months ago

Progress

About this first commit

Hey Damian, I am trying to implement in a clean way condstore for imap-codec. I wanted to start with a scope as small as possible, that's why I have a commit adding support only for the status codes used by condstore. I propose you review this commit in depth, pinpoint all the problems with your conventions/standard, so I can backport the rest in a proper way?

Some questions

As for now, I am validating my code with:

cargo build --features ext_condstore_qresync
cargo test --features ext_condstore_qresync
cargo clippy
cargo fmt

Is it ok? Is it normal that I have warnings when running cargo fmt? Do you know why the imap-types/src/core.rs file has been impacted by my cargo fmt despite the fact that I have not edited it?

Also, another question: I created a mod_sequence_value parser that use your number64 parser. I did not name it nz_number64 despite the same logic for NonZeroU32 being name nz_number as it seems you want to follow the ABNF grammar, and in the grammar, it's called mod-sequence-value. Am I correct?

Also, you said at FOSDEM '24 that often examples given in the RFC are wrong. It appears that HIGHESTMODSEQ is given without a text following the code. Reading your code let me guess that's wrong. For example:

      S: * 1 RECENT
      S: * OK [UNSEEN 12] Message 12 is first unseen
      S: * OK [UIDVALIDITY 3857529045] UIDs valid
      S: * OK [UIDNEXT 4392] Predicted next UID
      S: * FLAGS (\Answered \Flagged \Deleted \Seen \Draft)
      S: * OK [PERMANENTFLAGS (\Deleted \Seen \*)] Limited
      S: * OK [HIGHESTMODSEQ 715194045007]
      S: A142 OK [READ-WRITE] SELECT completed

Dovecot correctly adds the text Highest after the code:

      S: * OK [HIGHESTMODSEQ 715194045007] Highest

I don't know if it qualifies for your IMAP defects repository. Also feel free to edit this pull request directly if needed.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 7827660241


Changes Missing Coverage Covered Lines Changed/Added Lines %
imap-codec/src/codec/encode.rs 0 10 0.0%
<!-- Total: 28 38 73.68% -->
Files with Coverage Reduction New Missed Lines %
imap-codec/src/codec/encode.rs 1 89.71%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 7784490136: -0.07%
Covered Lines: 9574
Relevant Lines: 10296

💛 - Coveralls
superboum commented 4 months ago

It appears that cargo fmt and most commands should be run with the nightly toolchain. For the record, on NixOS, I don't use rustup, a nightly toolchain can be obtained with the following shell.nix:

{ pkgs ? import <nixpkgs> {} }:
let
  fenix = import (fetchTarball "https://github.com/nix-community/fenix/archive/main.tar.gz") { };
in
pkgs.mkShell {
  # nativeBuildInputs is usually what you want -- tools you need to run
  nativeBuildInputs = with pkgs.buildPackages; [
    fenix.complete.toolchain
  ];
}

So now formatting should pass hopefully.

duesee commented 4 months ago

It appears that cargo fmt and most commands should be run with the nightly toolchain.

It should generally only be cargo +nightly fmt (due to some options in rustfmt.toml) and fuzzing. Everything else is expected to work on stable.

For the record, on NixOS, I don't use rustup, a nightly toolchain can be obtained with the following shell.nix:

Do you think it would be beneficial to add this file to the repository? I'm open to anything that could improve the life of our contributors :-) Maybe we should add a snippet in CONTRIBUTING.md as well ...