Repository: aisk/rust-memcache (language: rust)

Issue:
Take values by reference in Client methods
The methods defined on `Client` like `gets`, `set` etc. take ownership of values, but there is no real reason to do so. Instead, they can take `V`, `Vec` by reference so that the values can be re-used.

The only downside is that this is a breaking change.


Discussion / hints:

Fix patch:
diff --git a/src/client.rs b/src/client.rs
index 15fdc35..13087ec 100644
--- a/src/client.rs
+++ b/src/client.rs
@@ -193,11 +193,11 @@ impl Client {
     /// ```rust
     /// let mut client = memcache::Client::connect("memcache://localhost:12345").unwrap();
     /// client.set("foo", "42", 0).unwrap();
-    /// let result: std::collections::HashMap<String, String> = client.gets(vec!["foo", "bar", "baz"]).unwrap();
+    /// let result: std::collections::HashMap<String, String> = client.gets(&["foo", "bar", "baz"]).unwrap();
     /// assert_eq!(result.len(), 1);
     /// assert_eq!(result["foo"], "42");
     /// ```
-    pub fn gets<V: FromMemcacheValueExt>(&mut self, keys: Vec<&str>) -> Result<HashMap<String, V>, MemcacheError> {
+    pub fn gets<V: FromMemcacheValueExt>(&mut self, keys: &[&str]) -> Result<HashMap<String, V>, MemcacheError> {
         let mut con_keys: HashMap<usize, Vec<&str>> = HashMap::new();
         let mut result: HashMap<String, V> = HashMap::new();
         let connections_count = self.connections.len();
@@ -209,7 +209,7 @@ impl Client {
         }
         for (&connection_index, keys) in con_keys.iter() {
             let connection = &mut self.connections[connection_index];
-            result.extend(connection.protocol.gets(keys.to_vec())?);
+            result.extend(connection.protocol.gets(keys)?);
         }
         return Ok(result);
     }
@@ -240,7 +240,7 @@ impl Client {
     /// use std::collections::HashMap;
     /// let mut client = memcache::Client::connect("memcache://localhost:12345").unwrap();
     /// client.set("foo", "bar", 10).unwrap();
-    /// let result: HashMap<String, (Vec<u8>, u32, Option<u64>)> = client.gets(vec!["foo"]).unwrap();
+    /// let result: HashMap<String, (Vec<u8>, u32, Option<u64>)> = client.gets(&["foo"]).unwrap();
     /// let (_, _, cas) = result.get("foo").unwrap();
     /// let cas = cas.unwrap();
     /// assert_eq!(true, client.cas("foo", "bar2", 10, cas).unwrap());
diff --git a/src/protocol/ascii.rs b/src/protocol/ascii.rs
index cd62157..58cb004 100644
--- a/src/protocol/ascii.rs
+++ b/src/protocol/ascii.rs
@@ -190,10 +190,7 @@ impl AsciiProtocol<Stream> {
         return Ok(Some(FromMemcacheValueExt::from_memcache_value(buffer, flags, None)?));
     }
 
-    pub(super) fn gets<V: FromMemcacheValueExt>(
-        &mut self,
-        keys: Vec<&str>,
-    ) -> Result<HashMap<String, V>, MemcacheError> {
+    pub(super) fn gets<V: FromMemcacheValueExt>(&mut self, keys: &[&str]) -> Result<HashMap<String, V>, MemcacheError> {
         write!(self.reader.get_mut(), "gets {}\r\n", keys.join(" "))?;
 
         let mut result: HashMap<String, V> = HashMap::new();
diff --git a/src/protocol/binary.rs b/src/protocol/binary.rs
index 231ce5b..a0da667 100644
--- a/src/protocol/binary.rs
+++ b/src/protocol/binary.rs
@@ -131,10 +131,7 @@ impl BinaryProtocol {
         return binary_packet::parse_get_response(&mut self.stream);
     }
 
-    pub(super) fn gets<V: FromMemcacheValueExt>(
-        &mut self,
-        keys: Vec<&str>,
-    ) -> Result<HashMap<String, V>, MemcacheError> {
+    pub(super) fn gets<V: FromMemcacheValueExt>(&mut self, keys: &[&str]) -> Result<HashMap<String, V>, MemcacheError> {
         for key in keys {
             if key.len() > 250 {
                 return Err(MemcacheError::ClientError(String::from("key is too long")));
diff --git a/src/protocol/mod.rs b/src/protocol/mod.rs
index 0d68aac..77e5664 100644
--- a/src/protocol/mod.rs
+++ b/src/protocol/mod.rs
@@ -24,7 +24,7 @@ pub trait ProtocolTrait {
     fn flush(&mut self) -> Result<(), MemcacheError>;
     fn flush_with_delay(&mut self, delay: u32) -> Result<(), MemcacheError>;
     fn get<V: FromMemcacheValueExt>(&mut self, key: &str) -> Result<Option<V>, MemcacheError>;
-    fn gets<V: FromMemcacheValueExt>(&mut self, keys: Vec<&str>) -> Result<HashMap<String, V>, MemcacheError>;
+    fn gets<V: FromMemcacheValueExt>(&mut self, keys: &[&str]) -> Result<HashMap<String, V>, MemcacheError>;
     fn set<V: ToMemcacheValue<Stream>>(&mut self, key: &str, value: V, expiration: u32) -> Result<(), MemcacheError>;
     fn cas<V: ToMemcacheValue<Stream>>(
         &mut self,
diff --git a/src/value.rs b/src/value.rs
index a053e48..b399ed3 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -32,6 +32,20 @@ impl<'a, W: Write> ToMemcacheValue<W> for &'a [u8] {
     }
 }
 
+impl<'a, W: Write> ToMemcacheValue<W> for &'a String {
+    fn get_flags(&self) -> u32 {
+        ToMemcacheValue::<W>::get_flags(*self)
+    }
+
+    fn get_length(&self) -> usize {
+        ToMemcacheValue::<W>::get_length(*self)
+    }
+
+    fn write_to(&self, stream: &mut W) -> io::Result<()> {
+        ToMemcacheValue::<W>::write_to(*self, stream)
+    }
+}
+
 impl<W: Write> ToMemcacheValue<W> for String {
     fn get_flags(&self) -> u32 {
         return Flags::Bytes as u32;


Test patch:
diff --git a/tests/test_ascii.rs b/tests/test_ascii.rs
index a965366..7e77bb7 100644
--- a/tests/test_ascii.rs
+++ b/tests/test_ascii.rs
@@ -16,8 +16,7 @@ fn test_ascii() {
     assert_eq!(value, Some("bar".into()));
 
     client.set("ascii_baz", "qux", 0).unwrap();
-    let values: HashMap<String, (Vec<u8>, u32)> =
-        client.gets(vec!["ascii_foo", "ascii_baz", "not_exists_key"]).unwrap();
+    let values: HashMap<String, (Vec<u8>, u32)> = client.gets(&["ascii_foo", "ascii_baz", "not_exists_key"]).unwrap();
     assert_eq!(values.len(), 2);
     let ascii_foo_value = values.get("ascii_foo").unwrap();
     let ascii_baz_value = values.get("ascii_baz").unwrap();
diff --git a/tests/tests.rs b/tests/tests.rs
index ca24dc4..490288d 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -146,7 +146,7 @@ fn udp_test() {
     assert_eq!(client.touch("fooo", 12345).unwrap(), true);
 
     // gets is not supported for udp
-    let value: Result<std::collections::HashMap<String, String>, _> = client.gets(vec!["foo", "fooo"]);
+    let value: Result<std::collections::HashMap<String, String>, _> = client.gets(&["foo", "fooo"]);
     assert_eq!(value.is_ok(), false);
 
     let mut keys: Vec<String> = Vec::new();
@@ -171,30 +171,30 @@ fn udp_test() {
             let mut client = memcache::Client::connect("memcache://localhost:22345?udp=true").unwrap();
             for j in 0..50 {
                 let value = format!("{}{}", value, j);
-                client.set(key.as_str(), value.clone(), 0).unwrap();
+                client.set(key.as_str(), &value, 0).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
-                assert_eq!(result, Some(value.clone()));
+                assert_eq!(result.as_ref(), Some(&value));
 
-                let result = client.add(key.as_str(), value.clone(), 0);
+                let result = client.add(key.as_str(), &value, 0);
                 assert_eq!(result.is_err(), true);
 
                 client.delete(key.as_str()).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
                 assert_eq!(result, None);
 
-                client.add(key.as_str(), value.clone(), 0).unwrap();
+                client.add(key.as_str(), &value, 0).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
-                assert_eq!(result, Some(value.clone()));
+                assert_eq!(result.as_ref(), Some(&value));
 
-                client.replace(key.as_str(), value.clone(), 0).unwrap();
+                client.replace(key.as_str(), &value, 0).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
-                assert_eq!(result, Some(value.clone()));
+                assert_eq!(result.as_ref(), Some(&value));
 
-                client.append(key.as_str(), value.clone()).unwrap();
+                client.append(key.as_str(), &value).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
                 assert_eq!(result, Some(format!("{}{}", value, value)));
 
-                client.prepend(key.as_str(), value.clone()).unwrap();
+                client.prepend(key.as_str(), &value).unwrap();
                 let result: Option<String> = client.get(key.as_str()).unwrap();
                 assert_eq!(result, Some(format!("{}{}{}", value, value, value)));
             }
@@ -224,7 +224,7 @@ fn test_cas() {
         client.set("ascii_baz", "qux", 0).unwrap();
 
         let values: HashMap<String, (Vec<u8>, u32, Option<u64>)> =
-            client.gets(vec!["ascii_foo", "ascii_baz", "not_exists_key"]).unwrap();
+            client.gets(&["ascii_foo", "ascii_baz", "not_exists_key"]).unwrap();
         assert_eq!(values.len(), 2);
         let ascii_foo_value = values.get("ascii_foo").unwrap();
         let ascii_baz_value = values.get("ascii_baz").unwrap();