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
Replace mpsc::{Sender,Receiver} by VecDeque #260
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
Additional details and impacted files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to make this change! And sorry for the slow reply 😅
Just one bit in there that I think is wrong, otherwise looks good 👍
/// Server responses that are not related to the current command. See also the note on | ||
/// [unilateral server responses in RFC 3501](https://tools.ietf.org/html/rfc3501#section-7). | ||
pub unsolicited_responses: mpsc::Receiver<UnsolicitedResponse>, | ||
pub unsolicited_responses: VecDeque<UnsolicitedResponse>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case we want to change this again in the future, I wonder if we should stop having this be a pub
field, and instead have a take_unsolicited()
method, or something like it? Maybe:
impl Session {
pub fn next_unsolicited(&mut self) -> Option<UnsolicitedResponse>;
pub fn all_unsolicited(&mut self) -> Vec<UnsolicitedResponse>;
}
That way the exact type we use here isn't exposed in the API, and thus can be changed. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even:
pub fn all_unsolicited(&mut self) -> impl Iterator<Item = UnsolicitedResponse> {
std::mem::take(&mut self.unsolicited_responses).into_iter()
}
Moving out of the field safes us an additional allocation of a Vec
, while still hiding the concrete queue-type from the public interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works too, though maybe -> impl ExactSizeIterator
to be even nicer to the caller :)
@@ -354,7 +349,7 @@ pub fn parse_mailbox( | |||
} | |||
Ok((rest, Response::Expunge(n))) => { | |||
lines = rest; | |||
unsolicited.send(UnsolicitedResponse::Expunge(n)).unwrap(); | |||
unsolicited.push_front(UnsolicitedResponse::Expunge(n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a push_back
, no?
Hi @soywod I really like this PR and it also looks super close to being finished. Is there anyway I can help get this merged? |
I forgot about this PR as it did not fix my initial issue. I can take a look back during this week and see if I can complete it. |
Fixes #254.
This change is