Implement changes & suggestions by @jonhoo

Author: dequbed

src/client.rs

@@ -139,7 +139,7 @@ fn validate_sequence_set(
 pub struct Session<T: Read + Write> {
     conn: Connection<T>,
 
-    // Capabilities are almost guaranteed to chance if encryption state or authentication state
+    // Capabilities are almost guaranteed to change if encryption state or authentication state
     // changes, so caching them in `Connection` is inappropiate.
     capability_cache: Option<Capabilities>,
 
@@ -159,7 +159,7 @@ pub struct Session<T: Read + Write> {
 pub struct Client<T: Read + Write> {
     conn: Connection<T>,
 
-    // Capabilities are almost guaranteed to chance if encryption state or authentication state
+    // Capabilities are almost guaranteed to change if encryption state or authentication state
     // changes, so caching them in `Connection` is inappropiate.
     capability_cache: Option<Capabilities>,
 }
@@ -362,16 +362,16 @@ impl<T: Read + Write> Client<T> {
     /// This method will always bypass the local capabilities cache and send a `CAPABILITY` command
     /// to the server. The [`Self::capabilities()`] method can be used when returning a cached
     /// response is acceptable.
-    pub fn capabilities_refresh(&mut self) -> Result<&Capabilities> {
+    pub fn current_capabilities(&mut self) -> Result<&Capabilities> {
         let (mut tx, _rx) = mpsc::channel();
         let caps = self.run_command_and_read_response("CAPABILITY")
             .and_then(|lines| Capabilities::parse(lines, &mut tx))?;
         self.capability_cache = Some(caps);
 
-        self.capability_cache.as_ref()
+        Ok(self.capability_cache.as_ref()
             // This path will not be hit; if the cache is not populated the above calls will either
             // populate it or return with an early error.
-            .ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
+            .expect("CAPABILITY call did not populate capability cache!"))
     }
 
     /// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
@@ -381,7 +381,7 @@ impl<T: Read + Write> Client<T> {
     /// This function will not query the server if a set of capabilities was cached, but request
     /// and cache capabilities from the server otherwise. The [`Self::capabilities_refresh`] method
     /// can be used to refresh the cache by forcing a `CAPABILITY` command to be send.
-    pub fn capabilities_ref(&mut self) -> Result<&Capabilities> {
+    pub fn capabilities(&mut self) -> Result<&Capabilities> {
         let (mut tx, _rx) = mpsc::channel();
         if self.capability_cache.is_none() {
             let caps = self.run_command_and_read_response("CAPABILITY")
@@ -390,10 +390,10 @@ impl<T: Read + Write> Client<T> {
             self.capability_cache = Some(caps);
         }
 
-        self.capability_cache.as_ref()
+        Ok(self.capability_cache.as_ref()
             // This path will not be hit; if the cache is not populated the above `if` will either
             // populate it or return with an early error.
-            .ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
+            .expect("CAPABILITY call did not populate capability cache!"))
     }
 
     /// Log in to the IMAP server. Upon success a [`Session`](struct.Session.html) instance is
@@ -830,7 +830,7 @@ impl<T: Read + Write> Session<T> {
     /// This method will always bypass the local capabilities cache and send a `CAPABILITY` command
     /// to the server. The [`Self::capabilities()`] method can be used when returning a cached
     /// response is acceptable.
-    pub fn capabilities_refresh(&mut self) -> Result<&Capabilities> {
+    pub fn current_capabilities(&mut self) -> Result<&Capabilities> {
         let caps = self.run_command_and_read_response("CAPABILITY")
             .and_then(|lines| Capabilities::parse(lines, &mut self.unsolicited_responses_tx))?;
         self.capability_cache = Some(caps);
@@ -848,7 +848,7 @@ impl<T: Read + Write> Session<T> {
     /// This function will not query the server if a set of capabilities was cached, but request
     /// and cache capabilities from the server otherwise. The [`Self::capabilities_refresh`] method
     /// can be used to refresh the cache by forcing a `CAPABILITY` command to be send.
-    pub fn capabilities_ref(&mut self) -> Result<&Capabilities> {
+    pub fn capabilities(&mut self) -> Result<&Capabilities> {
         if self.capability_cache.is_none() {
             let caps = self.run_command_and_read_response("CAPABILITY")
                 .and_then(|lines| Capabilities::parse(lines, &mut self.unsolicited_responses_tx))?;
@@ -862,15 +862,6 @@ impl<T: Read + Write> Session<T> {
             .ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
     }
 
-    /// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
-    /// listing of capabilities that the server supports.  The server will include "IMAP4rev1" as
-    /// one of the listed capabilities. See [`Capabilities`] for further details.
-    pub fn capabilities(&mut self) -> Result<Capabilities> {
-        // TODO: This emulated the same behaviour as before, with each call issuing a command.
-        //       It may be sensible to allow hitting the cache here.
-        self.capabilities_refresh().map(|caps| caps.clone())
-    }
-
     /// The [`EXPUNGE` command](https://tools.ietf.org/html/rfc3501#section-6.4.3) permanently
     /// removes all messages that have [`Flag::Deleted`] set from the currently selected mailbox.
     /// The message sequence number of each message that is removed is returned.
@@ -2826,7 +2817,7 @@ a1 OK completed\r
         ];
         let mock_stream = MockStream::new(response);
         let mut session = mock_session!(mock_stream);
-        let capabilities = session.capabilities().unwrap();
+        let capabilities = session.capabilities().cloned().unwrap();
         assert!(
             session.stream.get_ref().written_buf == b"a1 CAPABILITY\r\n".to_vec(),
             "Invalid capability command"

src/types/capabilities.rs

@@ -48,7 +48,8 @@ impl Clone for Capabilities {
     fn clone(&self) -> Self {
         // Give _rx a name so it's not immediately dropped. Otherwise any unsolicited responses
         // that would be send there will return a SendError instead of the parsed response simply
-        // being dropped later.
+        // being dropped later. Those responses being dropped is safe as they were already parsed
+        // and sent to a consumer when this capabilities response was parsed the first time.
         let (mut tx, _rx) = mpsc::channel();
         Self::parse(self.borrow_data().clone(), &mut tx)
             .expect("failed to parse capabilities from data which was already successfully parse before")