duesee / imap-codec

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

refactor: Redesign `Status` #372

Closed duesee closed 8 months ago

duesee commented 8 months ago

Working on a higher-level implementation shows that another definition of Status would be more appropriate.

Mostly, we are first interested if we are dealing with an untagged status, i.e., an alternative form of server data, or a tagged status, i.e., a command completion response. Thus, Untagged, and Done are matched first on the highest level. Bye should be handled here, too.

This representation makes more sense in practice:

#[derive(Debug)]
pub enum StatusV2<'a> {
    Untagged(StatusBody<'a>),
    Tagged(Tagged<'a>),
    Bye(Bye<'a>),
}

#[derive(Debug)]
pub struct StatusBody<'a> {
    pub kind: StatusKind,
    pub code: Option<Code<'a>>,
    pub text: Text<'a>,
}

#[derive(Debug)]
pub enum StatusKind {
    Ok,
    No,
    Bad,
}

#[derive(Debug)]
pub struct Tagged<'a> {
    pub tag: Tag<'a>,
    pub body: StatusBody<'a>,
}

#[derive(Debug)]
pub struct Bye<'a> {
    pub code: Option<Code<'a>>,
    pub text: Text<'a>,
}

We should introduce a leightweight conversion w/o a breaking change.

Edit: Improved definition a bit.

mamaicode commented 8 months ago

Could you please clarify a bit more what needs to be implemented exactly?

duesee commented 8 months ago

Sure! Given that the new type weights it's merit, we probably want to ...

Note: The old type is still absolutely fine. It's just that pattern matching would be way easier with the new one. If, at some later point in time, we want to release imap-codec 2.0.0, we would remove the old Status.

duesee commented 8 months ago

Further explanation: I have a (not to be merged in this way) branch here, https://github.com/duesee/imap-flow/blob/duesee_cli/client/src/lib.rs#L400, and suspect this (typical?) code could be way simpler with a slightly different Status enum. The repeated match-gates could go away, I think.

duesee commented 8 months ago

Upon further thought, this might not be a very good first issue as we probably want to rethink Response as well when we are at it. Due to this uncertainty, I removed the label for now, although having a StatusV2 would already be helpful.

mamaicode commented 8 months ago

Sorry I did not continue to work on this, the issue seemed a bit more complex and in order to implement I need to get more familiar with the code!! My time is a bit limited at the moment :(

duesee commented 8 months ago

No worries! Happy you considered helping! :-)

For this issue, I think I'll try to bring something in place soon. Even though, it might not be the final thing we eventually want to release.