Add clippy to CI + address clippy problems (#215)

* Add clippy to CI + address clippy problems

This commit
1. adds `cargo clippy` to .travis.yml
2. addresses clippy concerns and clarifies places where they should be ignored.

* Add clippy component in travis CI

* Fix from_str errors caused by a clippy fix

The issue was that the from_str implementation on Value was moved to the
FromStr trait as recommended by Clippy. This caused many of the tests to
fail since now the FromStr trait must be in scope. Instead of bringing
FromStr into scope everywhere, however, the `Value::from_str(_)` calls
were replaced with `_.parse()` calls as recommended by the std lib
documentation.

* Format crate + rename CI targets for clarity

.travis.yml

@@ -11,14 +11,23 @@ branches:
     - master
 
 matrix:
+  allow_failures:
+    - name: "Clippy: stable"
   include:
     - rust: stable
+      name: "Format: stable"
       script:
         - rustup component add rustfmt
         - cargo fmt -- --check
+    - rust: stable
+      name: "Clippy: stable"
+      script:
+        - rustup component add clippy
+        - cargo clippy
 
 cache: cargo
 
 script:
   - cargo test
   - cargo test --all-features
+

examples/transcode.rs

@@ -25,7 +25,7 @@ fn main() {
         )
         "#;
 
-    let value = Value::from_str(data).expect("Failed to deserialize");
+    let value: Value = data.parse().expect("Failed to deserialize");
     let mut ser = serde_json::Serializer::pretty(std::io::stdout());
     value.serialize(&mut ser).expect("Failed to serialize");
 }

src/de/mod.rs

@@ -26,6 +26,8 @@ pub struct Deserializer<'de> {
 }
 
 impl<'de> Deserializer<'de> {
+    // Cannot implement trait here since output is tied to input lifetime 'de.
+    #[allow(clippy::should_implement_trait)]
     pub fn from_str(input: &'de str) -> Result<Self> {
         Deserializer::from_bytes(input.as_bytes())
     }
@@ -102,18 +104,18 @@ impl<'de> Deserializer<'de> {
         // Create a working copy
         let mut bytes = self.bytes;
 
-        match bytes.consume("(") {
-            true => {
-                bytes.skip_ws()?;
+        if bytes.consume("(") {
+            bytes.skip_ws()?;
 
-                match bytes.check_tuple_struct()? {
-                    // first argument is technically incorrect, but ignored anyway
-                    true => self.deserialize_tuple(0, visitor),
-                    // first two arguments are technically incorrect, but ignored anyway
-                    false => self.deserialize_struct("", &[], visitor),
-                }
+            if bytes.check_tuple_struct()? {
+                // first argument is technically incorrect, but ignored anyway
+                self.deserialize_tuple(0, visitor)
+            } else {
+                // first two arguments are technically incorrect, but ignored anyway
+                self.deserialize_struct("", &[], visitor)
             }
-            false => visitor.visit_unit(),
+        } else {
+            visitor.visit_unit()
         }
     }
 }

src/de/value.rs

@@ -10,9 +10,11 @@ use crate::{
     value::{Map, Number, Value},
 };
 
-impl Value {
+impl std::str::FromStr for Value {
+    type Err = de::Error;
+
     /// Creates a value from a string reference.
-    pub fn from_str(s: &str) -> de::Result<Self> {
+    fn from_str(s: &str) -> de::Result<Self> {
         let mut de = super::Deserializer::from_str(s)?;
 
         let val = Value::deserialize(&mut de)?;
@@ -166,9 +168,10 @@ impl<'de> Visitor<'de> for ValueVisitor {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::str::FromStr;
 
     fn eval(s: &str) -> Value {
-        Value::from_str(s).expect("Failed to parse")
+        s.parse().expect("Failed to parse")
     }
 
     #[test]

src/parse.rs

@@ -155,6 +155,9 @@ impl<'a> Bytes<'a> {
     }
 
     pub fn any_num(&mut self) -> Result<AnyNum> {
+        // We are not doing float comparisons here in the traditional sense.
+        // Instead, this code checks if a f64 fits inside an f32.
+        #[allow(clippy::float_cmp)]
         fn any_float(f: f64) -> Result<AnyNum> {
             if f == f as f32 as f64 {
                 Ok(AnyNum::F32(f as f32))
@@ -378,10 +381,8 @@ impl<'a> Bytes<'a> {
     }
 
     pub fn expect_byte(&mut self, byte: u8, error: ParseError) -> Result<()> {
-        self.eat_byte().and_then(|b| match b == byte {
-            true => Ok(()),
-            false => self.err(error),
-        })
+        self.eat_byte()
+            .and_then(|b| if b == byte { Ok(()) } else { self.err(error) })
     }
 
     /// Returns the extensions bit mask.
@@ -424,9 +425,10 @@ impl<'a> Bytes<'a> {
 
         self.skip_ws()?;
 
-        match self.consume_all(&[")", "]"])? {
-            true => Ok(extensions),
-            false => Err(self.error(ParseError::ExpectedAttributeEnd)),
+        if self.consume_all(&[")", "]"])? {
+            Ok(extensions)
+        } else {
+            Err(self.error(ParseError::ExpectedAttributeEnd))
         }
     }
 
@@ -714,9 +716,8 @@ impl<'a> Bytes<'a> {
                 }
 
                 self.expect_byte(b'}', ParseError::InvalidEscape("No } at the end"))?;
-                let character = char_from_u32(bytes)
-                    .ok_or_else(|| self.error(ParseError::InvalidEscape("Not a valid char")))?;
-                character
+                char_from_u32(bytes)
+                    .ok_or_else(|| self.error(ParseError::InvalidEscape("Not a valid char")))?
             }
             _ => {
                 return self.err(ParseError::InvalidEscape("Unknown escape character"));

src/value.rs

@@ -153,10 +153,7 @@ impl Number {
 /// underlying `f64` values itself.
 impl PartialEq for Number {
     fn eq(&self, other: &Self) -> bool {
-        if self.0.is_nan() && other.0.is_nan() {
-            return true;
-        }
-        return self.0 == other.0;
+        self.0.is_nan() && other.0.is_nan() || self.0 == other.0
     }
 }
 

tests/value.rs

@@ -3,61 +3,55 @@ use serde::Serialize;
 
 #[test]
 fn bool() {
-    assert_eq!(Value::from_str("true"), Ok(Value::Bool(true)));
-    assert_eq!(Value::from_str("false"), Ok(Value::Bool(false)));
+    assert_eq!("true".parse(), Ok(Value::Bool(true)));
+    assert_eq!("false".parse(), Ok(Value::Bool(false)));
 }
 
 #[test]
 fn char() {
-    assert_eq!(Value::from_str("'a'"), Ok(Value::Char('a')));
+    assert_eq!("'a'".parse(), Ok(Value::Char('a')));
 }
 
 #[test]
 fn map() {
     let mut map = Map::new();
     map.insert(Value::Char('a'), Value::Number(Number::new(1f64)));
     map.insert(Value::Char('b'), Value::Number(Number::new(2f64)));
-    assert_eq!(Value::from_str("{ 'a': 1, 'b': 2 }"), Ok(Value::Map(map)));
+    assert_eq!("{ 'a': 1, 'b': 2 }".parse(), Ok(Value::Map(map)));
 }
 
 #[test]
 fn number() {
-    assert_eq!(Value::from_str("42"), Ok(Value::Number(Number::new(42f64))));
-    assert_eq!(
-        Value::from_str("3.1415"),
-        Ok(Value::Number(Number::new(3.1415f64)))
-    );
+    assert_eq!("42".parse(), Ok(Value::Number(Number::new(42f64))));
+    assert_eq!("3.1415".parse(), Ok(Value::Number(Number::new(3.1415f64))));
 }
 
 #[test]
 fn option() {
     let opt = Some(Box::new(Value::Char('c')));
-    assert_eq!(Value::from_str("Some('c')"), Ok(Value::Option(opt)));
+    assert_eq!("Some('c')".parse(), Ok(Value::Option(opt)));
 }
 
 #[test]
 fn string() {
     let normal = "\"String\"";
-    assert_eq!(Value::from_str(normal), Ok(Value::String("String".into())));
+    assert_eq!(normal.parse(), Ok(Value::String("String".into())));
 
     let raw = "r\"Raw String\"";
-    assert_eq!(Value::from_str(raw), Ok(Value::String("Raw String".into())));
+    assert_eq!(raw.parse(), Ok(Value::String("Raw String".into())));
 
     let raw_hashes = "r#\"Raw String\"#";
-    assert_eq!(
-        Value::from_str(raw_hashes),
-        Ok(Value::String("Raw String".into()))
-    );
+    assert_eq!(raw_hashes.parse(), Ok(Value::String("Raw String".into())));
 
     let raw_escaped = "r##\"Contains \"#\"##";
     assert_eq!(
-        Value::from_str(raw_escaped),
+        raw_escaped.parse(),
         Ok(Value::String("Contains \"#".into()))
     );
 
     let raw_multi_line = "r\"Multi\nLine\"";
     assert_eq!(
-        Value::from_str(raw_multi_line),
+        raw_multi_line.parse(),
         Ok(Value::String("Multi\nLine".into()))
     );
 }
@@ -68,18 +62,18 @@ fn seq() {
         Value::Number(Number::new(1f64)),
         Value::Number(Number::new(2f64)),
     ];
-    assert_eq!(Value::from_str("[1, 2]"), Ok(Value::Seq(seq)));
+    assert_eq!("[1, 2]".parse(), Ok(Value::Seq(seq)));
 }
 
 #[test]
 fn unit() {
     use ron::de::{Error, ParseError, Position};
 
-    assert_eq!(Value::from_str("()"), Ok(Value::Unit));
-    assert_eq!(Value::from_str("Foo"), Ok(Value::Unit));
+    assert_eq!("()".parse(), Ok(Value::Unit));
+    assert_eq!("Foo".parse(), Ok(Value::Unit));
 
     assert_eq!(
-        Value::from_str(""),
+        "".parse::<Value>(),
         Err(Error::Parser(ParseError::Eof, Position { col: 1, line: 1 }))
     );
 }