Repository: ron-rs/ron
Pull request #215 (2020-04-21 02:05:23)

Add clippy to CI + address clippy problems
This commit
1. Adds `cargo clippy` to .travis.yml that is allowed to fail to reduce friction for incoming PRs.
2. Addresses clippy concerns and clarifies places where they should be ignored.

Addresses half of #211

elrnv: I think I had a `rustup component add clippy` missing here and same goes for #214. I updated both, hopefully this will satisfy the CI.
kvark: Please rebase!
elrnv: All done!

Patch:
diff --git a/.travis.yml b/.travis.yml
index 2cefb56a..177f0224 100644
--- a/.travis.yml
+++ b/.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
+
diff --git a/examples/transcode.rs b/examples/transcode.rs
index 401f6512..115ac469 100644
--- a/examples/transcode.rs
+++ b/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");
 }
diff --git a/src/de/mod.rs b/src/de/mod.rs
index 71bfd826..0cad6fb5 100644
--- a/src/de/mod.rs
+++ b/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()
         }
     }
 }
diff --git a/src/de/value.rs b/src/de/value.rs
index 7e07c628..da4c6176 100644
--- a/src/de/value.rs
+++ b/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]
diff --git a/src/parse.rs b/src/parse.rs
index 835e9227..309ec82e 100644
--- a/src/parse.rs
+++ b/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"));
diff --git a/src/value.rs b/src/value.rs
index 2fafa130..a848c9f2 100644
--- a/src/value.rs
+++ b/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
     }
 }
 


Test patch:
diff --git a/tests/value.rs b/tests/value.rs
index 6180e597..05b155ff 100644
--- a/tests/value.rs
+++ b/tests/value.rs
@@ -3,13 +3,13 @@ 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]
@@ -17,47 +17,41 @@ 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 }))
     );
 }