Skip to content

Implemented new enumerated Capabilities from imap-proto crate. #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

lberezy
Copy link
Contributor

@lberezy lberezy commented May 3, 2019

This PR exposes the new capability of the underlying imap-proto crate (djc/tokio-imap#47) to parse Capabilities into an enumerated type.

Note that this is not yet ready for merge as it depends on the master of imap-proto, not a currently released version.

self.0.contains(s)
}

/// Check if the server has the given capability via str.
pub fn has_str(&self, s: &str) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for this to be generic over the type of string the same way the old one was.

@jonhoo
Copy link
Owner

jonhoo commented May 3, 2019

Thank you! It's a little unfortunate that this is a breaking change as mentioned in the comments, but apart from that it looks pretty good!

@link2xt
Copy link

link2xt commented Aug 2, 2019

Note that this is not yet ready for merge as it depends on the master of imap-proto, not a currently released version.

imap-proto version 0.8.1 has been released recently, see djc/tokio-imap#52

@jonhoo
Copy link
Owner

jonhoo commented Aug 20, 2019

A gentle ping on this @lberezy.

@avitex
Copy link
Contributor

avitex commented Aug 20, 2019

@jonhoo i'll have look at this tomorrow :)

@avitex
Copy link
Contributor

avitex commented Aug 22, 2019

@jonhoo There are three solutions I can see going forward.

As the upstream Capability type cuts off the AUTH= prefix, so dealing with just &str becomes a little more troublesome.

  1. We create an owned Capability (requires copy of data as &str is contiguous) adding the prefix.
  2. The upstream crate is changed to not cut the prefix (a breaking change).
  3. This crate makes a breaking change.

All three are not desirable, that I understand. You've put good effort in making this crate with zero copy guarantees. I would make a wager you've already identified this dilemma, and would prefer upstream to change 😛

I'm happy to implement whichever case it be, but what in your eyes is the lesser evil?
I personally prefer a breaking change on this crate.

@lberezy
Copy link
Contributor Author

lberezy commented Aug 22, 2019

It might be painful, but I think I agree that picking number 3 might be the best option.

@jonhoo
Copy link
Owner

jonhoo commented Aug 22, 2019

Hmm, yeah, good points. Okay, I guess it's time to do a breaking change. It's probably good to have this coincide with #130, which is not quite a breaking change, but not far from it.

@jonhoo
Copy link
Owner

jonhoo commented Sep 1, 2019

I think the only thing missing here then is the one outstanding comment about making has_str generic over the string type the same way the old one was.

@jonhoo
Copy link
Owner

jonhoo commented Sep 2, 2019

Replaced by #133.

@jonhoo jonhoo closed this Sep 2, 2019
@jonhoo jonhoo mentioned this pull request Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants