Fix some UIDPLUS issues

[nevans]

• 🐛 Bug fixed: uid-range should behave the same, regardless of order. i.e. "2:4" and "4:2" are equivalent.
• 🐛 Reversed a breaking change: The return values of Net::IMAP#copy, #move, #uid_copy, #uid_move, and #append are incompatible with all previously released versions. That would break any apps that expect the tagged response. TaggedResponse is more broadly useful anyway, since it contains the response code with its data.
• Forward compatibility with MULTIAPPEND: I converted "append-uid" to parse a "uid-set" instead of a "uniqueid". This also provides a consistent interface: assigned_uids will always be an array of ints.
• Replaced arrays with a UIDPlusData struct. In my opinion, it's much nicer to use a Struct than a "tuple". This also provides a home for documentation and utility methods. E.g. I also added UIDPlusData#uid_mapping, which returns a hash of {src_uid => dst_uid}.
• Number parsing was converted from to_i to Integer. We matched the uid-set with a single T_ATOM token, so we haven't validated that it follows the rest of the grammar. This will still parse some invalid strings (e.g. ",:1,,:::,,"), but in practice it's much more likely that a bogus atom would have at least one non-numeric character besides ":" and ",". (Although I've seen some crazy server bugs...)

[nevans]

@hoffi would you mind taking a look at these changes to the new UIDPLUS code, please? Let me know what you think. Thanks!

@shugo I think we should fix these bugs before the next release. Especially the changed return values.

[hoffi]

Hi @nevans,

good catch with that uid-range ordering. Seems like I misunderstood the RFC that they are always in ascending order.
But that only refers to the assignment of the UIDs and not necessarily for the returned order in the range:

UIDs are assigned in strictly ascending order in the mailbox and UID ranges are as in [IMAP]; in particular, note that a range of 12:10 is exactly equivalent to 10:12 and refers to the sequence 10,11,12.

I really like the idea of the UIDPlusData struct. This gives a bit more context at what the data really is.

It would still be a breaking change for anyone that already parsed the information out of the response code data, as it is no longer a string but now a UIDPlusData struct.
Would this still be a problem? Maybe ResponseCode needs another field to hold both the raw data (the raw string from the IMAP response) and the parsed data (now the UIDPlusData)?
Edit: I just realised that making this change consistently would again bring breaking changes to BADCHARSET, CAPABILITY and PERMANENTFLAGS response codes.

[nevans]

It would still be a breaking change for anyone that already parsed the information out of the response code data, as it is no longer a string but now a UIDPlusData struct. Would this still be a problem? Maybe ResponseCode needs another field to hold both the raw data (the raw string from the IMAP response) and the parsed data (now the UIDPlusData)? Edit: I just realised that making this change consistently would again bring breaking changes to BADCHARSET, CAPABILITY and PERMANENTFLAGS response codes.

Yeah, I've thought about that a bit, too. I went through a lot of ideas, and my first thoughts were more-or-less what you suggested. And I'm sure I could think of some even more compatible approaches too. But my current thinking is much simpler:

Document that all response code data Strings are unparsed

If I remember correctly, I think that all response codes with data inside the [CODE data] square brackets have enough structure that we will never want to just return a string. Free-form strings should (and do) use ResponseText#text instead, e.g: "* [ALERT] this is the alert text!\r\n". So we can promise users: if Net::Imap parsed it, code.data will return an array or a struct or some other object, but never a string.

For convenience we might offer something like the following:

class ResponseCode
  def unparsed? = data.is_a?(String)
end

Document that there is absolutely no promises of forward compatibility for unparsed data

You use ResponseCode#data -> String at your own risk. If you are parsing the string yourself, upgrading from your parser to net-imap's should be relatively easy. We use fairly simple structs that usually mimic the ABNF grammar very closely (sometimes a little bit too closely). And we can encourage you to come here find (or create) an issue and post a comment. It doesn't need to be a full PR: even posting just a quick gist or regexp might get it done faster, and improves the odds that our implementation will be compatible with yours.

---

Alternatively, I've also been thinking about adding configuration and extension points in various places, using mixins, refinements, etc. E.g: I want to support an efficient "quirks mode" for different servers, so we don't need to pollute the main parser code with so many spec-violations. 🙂

---

What do you think?

[hoffi]

Yes seems fine to me to just document that Strings in the response code data represents unparsed data and that using them could result in future breaking changes.

Your other idea also sounds interesting, but don't think any of the current code would require something like that?

[shugo]

It looks fine. Than you!

[nevans]

thanks @hoffi and @shugo. I have some other minor rdoc tweaks I want to make, but I'll save those for a future PR.