Open superboum opened 5 months ago
Awesome! Do you want me to comment on the code already? :-)
Totals | |
---|---|
Change from base Build 7475751125: | -0.6% |
Covered Lines: | 9225 |
Relevant Lines: | 9944 |
In the end, I would be interested to know what is the proper way to implement features in imap_codec for sure! But I would be more comfortable with a progressive approach. The first step, according to me, seems to make sure that I am synchronized with the branch+code you want. I have commits from you that are not in main. Should I discard them?
I have commits from you that are not in main. Should I discard them?
refactor!: Replace StatusV2 with Status -> already on main, discard refactor(encoder)!: Add Fragment::AuthData -> discard (deleted from main) feat(imap-{types,codec})!: Implement AuthenticateData::Cancel -> already on main, discard quirk: mail.ru WIP -> already on main, discard
Ok so now I'm in sync with main and I have a single commit that contain only my changes: allowing SELECT/EXAMINE/STORE to have modifiers. Another FETCH modifier will come soon also ^^. Let me know if it's what you expect :-)
If everything is OK, the next step I envision to discuss with you is about your high-level guidelines to implement modifiers. Indeed, it seems that CONDSTORE introduce this novel concept of MODIFIERS in IMAP, concept that is described in a more generic way in RFC4466. So should we aim at being generic here? Or stay specific? The generic approach seems to be limited by the fact that the exact syntax of the modifiers value is not given, or at least I have not understood it. And the question following just this one: what shape has the imap-type you envision for these modifiers?
In the end, I would be interested to know what is the proper way to implement features in imap_codec for sure!
I don't know! 🙈 IMAP extensions are really more of "amendements"¹. It's difficult to reflect this in a library w/o breaking changes (or having a weird API).
This is our current approach:
When we add new extensions, we put them behind a feature flag starting with ext_
. This feature flag has basically only the rule that everything should work when it's not used. You can use it to add variants to an enum -- even when the enum is not #[non_exhaustive]
. This would normally be a breaking change but given that we document this features as experimental, we try to give us freedom here to experiment with features before we stabilize them. The expectation is that we will could have a few, say, 5 or so, ext_
features. And, the next time we do a major version, we'll "inline" the features into imap-{types,codec}
(removing the feature flag.)
¹ I stole this sentence from Simon (go-imap).
What shape has the imap-type you envision for these modifiers?
I'm not confident in this part of the spec yet. Let me just "think aloud".
If I understand correctly, we have ...
;; recommended overarching syntax for extensions
tagged-ext = tagged-ext-label SP tagged-ext-val
;; Is a valid RFC 3501 "atom".
tagged-ext-label = tagged-label-fchar *tagged-label-char
tagged-label-fchar = ALPHA / "-" / "_" / "."
tagged-label-char = tagged-label-fchar / DIGIT / ":"
tagged-ext-val = tagged-ext-simple / "(" [tagged-ext-comp] ")"
tagged-ext-simple = sequence-set / number
;; Extensions that follow this general syntax should use nstring instead of astring when appropriate in the context of the extension.
;; Note that a message set or a "number" can always be represented as an "atom".
;; An URL should be represented as a "quoted" string.
tagged-ext-comp = astring /
tagged-ext-comp *(SP tagged-ext-comp) /
"(" tagged-ext-comp ")"
... for the "generic approach", right?
And then, e.g., ...
fetch-modifiers = SP "(" fetch-modifier *(SP fetch-modifier) ")"
fetch-modifier = fetch-modifier-name [ SP fetch-modif-params ]
fetch-modifier-name = tagged-ext-label
;; This non-terminal shows recommended syntax for future extensions.
fetch-modif-params = tagged-ext-val
;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
fetch-modifier = tagged-ext-label [ SP tagged-ext-val ]
... and ...
esearch-response = "ESEARCH" [search-correlator] [SP "UID"] *(SP search-return-data)
;; Note that not every SEARCH return option is required to have the corresponding ESEARCH return data.
search-return-data = search-modifier-name SP search-return-value
search-modifier-name = tagged-ext-label
;; Data for the returned search option. A single "nz-number"/"number" value
;; can be returned as an atom (i.e., without quoting). A sequence-set can be
;; returned as an atom as well.
search-return-value = tagged-ext-val
;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
search-return-data = tagged-ext-label SP tagged-ext-val
... and ...
store-modifiers = SP "(" store-modifier *(SP store-modifier)")"
store-modifier = store-modifier-name [SP store-modif-params]
store-modifier-name = tagged-ext-label
;; This non-terminal shows recommended syntax for future extensions.
store-modif-params = tagged-ext-val
;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
store-modifier = tagged-ext-label [SP tagged-ext-val]
The weird part for me is:
Extensions that follow this general syntax should use nstring instead of astring when appropriate in the context of the extension.
Which means that some extensions do not follow the general syntax?! So I am not sure to understand the ins and the outs of implementing a generic approach that does not apply to all cases.
Which means that some extensions do not follow the general syntax?! So I am not sure to understand the ins and the outs of implementing a generic approach that does not apply to all cases.
Me neither. Accepting NString
in case of AString
would "hide" a possible Atom("nil")
?
So, it seems safe to implement tagged-ext-label
exactly as defined? Not sure about tagged-ext-val
due to the comment? (Havn't looked to deep in your code yet.) Maybe this is as "generic" as it gets :D)
Do you have an example of an extension that uses NString
instead?
Sorry, I should not edit my comments too fast.
Some issues that may come up when we implement generically... When you have number / atom
, you cannot tell what 123
should be. Currently, we try to test that parsing is the inverse of serialization. But when we naively use e.g. an enum, someone could construct an Atom("123")
, that will be parsed as Number(123)
breaking the inverse-property. This could be fun :-/
(If these cases are not easy to fix, I think we should give up on the inverse-test for some cases. Can't fix the standard ...)
Does the formal syntax only try to give an "idea" of how these things may look like? Is there a case when we really expect either a number or an atom? Seems weird? I think you are right in not being too optimistic about the generic approach.
Maybe we should not go for the generic modifier implementation? And only try to support modifiers for a selection of IMAP extensions like condstore? It seems that this generic specification is not very useful: another problem I face is that, while number are generally u32 in IMAP, CONDSTORE MODSEQ are u64, so when you use modifiers like CHANGEDSINCE XXX or UNCHANGED SINCE XXX, XXX is a u64. Or to be more specific, they can be u63 or u64 depending on which RFC you read x)
{,UN}CHANGEDSINCE is a new thing from CONDSTORE, right? Or is there a case where support of an extension requires to update existing syntax from u32 to u64?
Thanks for your review, indeed I am not testing the client encoding part as I am not using it yet. Thanks also for the input on Option
Don't worry! On my side, there is no time pressure regarding extensions, and I'm very happy about your contributions! Unfortunately, I don't know CONDSTORE/QRESYNC (yet) and am probably not too much of a help, sorry! But I'll jump in as soon as you have something reviewable (or ask for a review :-))!
I am currently working on implementing CONDSTORE in Aerogramme. You can track my work on the dedicated merge request thread. I am opening a PR so you can see which features I needed to add so the feature works. The code is ugly and is not meant to be merged.
Relevant RFC: 4466 + RFC7162