jingle_ibb, ibb, rsm: Simplify attribute parsing.

Emmanuel Gil Peyrot created 8 years ago

Change summary

src/ibb.rs        | 12 ++++++------
src/jingle_ibb.rs | 36 ++++++++++++++++--------------------
src/lib.rs        |  3 +++
src/rsm.rs        |  2 +-
4 files changed, 26 insertions(+), 27 deletions(-)

Detailed changes

src/ibb.rs

@@ -73,9 +73,9 @@ impl<'a> TryFrom<&'a Element> for IBB {
             for _ in elem.children() {
                 return Err(Error::ParseError("Unknown child in open element."));
             }
-            let block_size = get_attr!(elem, "block-size", required, block_size, block_size.parse()?);
-            let sid = get_attr!(elem, "sid", required, sid, sid.parse()?);
-            let stanza = get_attr!(elem, "stanza", default, stanza, stanza.parse()?);
+            let block_size = get_attr!(elem, "block-size", required);
+            let sid = get_attr!(elem, "sid", required);
+            let stanza = get_attr!(elem, "stanza", default);
             Ok(IBB::Open {
                 block_size: block_size,
                 sid: sid,
@@ -85,8 +85,8 @@ impl<'a> TryFrom<&'a Element> for IBB {
             for _ in elem.children() {
                 return Err(Error::ParseError("Unknown child in data element."));
             }
-            let seq = get_attr!(elem, "seq", required, seq, seq.parse()?);
-            let sid = get_attr!(elem, "sid", required, sid, sid.parse()?);
+            let seq = get_attr!(elem, "seq", required);
+            let sid = get_attr!(elem, "sid", required);
             let data = base64::decode(&elem.text())?;
             Ok(IBB::Data {
                 seq: seq,
@@ -97,7 +97,7 @@ impl<'a> TryFrom<&'a Element> for IBB {
             for _ in elem.children() {
                 return Err(Error::ParseError("Unknown child in close element."));
             }
-            let sid = get_attr!(elem, "sid", required, sid, sid.parse()?);
+            let sid = get_attr!(elem, "sid", required);
             Ok(IBB::Close {
                 sid: sid,
             })

src/jingle_ibb.rs

@@ -5,7 +5,6 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 use std::convert::TryFrom;
-use std::str::FromStr;
 
 use minidom::Element;
 
@@ -22,15 +21,6 @@ pub struct Transport {
     pub stanza: Stanza,
 }
 
-fn optional_attr<T: FromStr>(root: &Element, attr: &str) -> Option<T> {
-    root.attr(attr)
-        .and_then(|value| value.parse().ok())
-}
-
-fn required_attr<T: FromStr>(root: &Element, attr: &str, err: Error) -> Result<T, Error> {
-    optional_attr(root, attr).ok_or(err)
-}
-
 impl<'a> TryFrom<&'a Element> for Transport {
     type Error = Error;
 
@@ -39,11 +29,9 @@ impl<'a> TryFrom<&'a Element> for Transport {
             for _ in elem.children() {
                 return Err(Error::ParseError("Unknown child in JingleIBB element."));
             }
-            let block_size = required_attr(elem, "block-size", Error::ParseError("Required attribute 'block-size' missing in JingleIBB element."))?;
-            let sid = required_attr(elem, "sid", Error::ParseError("Required attribute 'sid' missing in JingleIBB element."))?;
-            let stanza = elem.attr("stanza")
-                             .unwrap_or("iq")
-                             .parse()?;
+            let block_size = get_attr!(elem, "block-size", required);
+            let sid = get_attr!(elem, "sid", required);
+            let stanza = get_attr!(elem, "stanza", default);
             Ok(Transport {
                 block_size: block_size,
                 sid: sid,
@@ -69,6 +57,7 @@ impl<'a> Into<Element> for &'a Transport {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::error::Error as StdError;
 
     #[test]
     fn test_simple() {
@@ -87,16 +76,23 @@ mod tests {
             Error::ParseError(string) => string,
             _ => panic!(),
         };
-        assert_eq!(message, "Required attribute 'block-size' missing in JingleIBB element.");
+        assert_eq!(message, "Required attribute 'block-size' missing.");
+
+        let elem: Element = "<transport xmlns='urn:xmpp:jingle:transports:ibb:1' block-size='65536'/>".parse().unwrap();
+        let error = Transport::try_from(&elem).unwrap_err();
+        let message = match error {
+            Error::ParseIntError(error) => error,
+            _ => panic!(),
+        };
+        assert_eq!(message.description(), "number too large to fit in target type");
 
-        // TODO: maybe make a better error message here.
         let elem: Element = "<transport xmlns='urn:xmpp:jingle:transports:ibb:1' block-size='-5'/>".parse().unwrap();
         let error = Transport::try_from(&elem).unwrap_err();
         let message = match error {
-            Error::ParseError(string) => string,
+            Error::ParseIntError(error) => error,
             _ => panic!(),
         };
-        assert_eq!(message, "Required attribute 'block-size' missing in JingleIBB element.");
+        assert_eq!(message.description(), "invalid digit found in string");
 
         let elem: Element = "<transport xmlns='urn:xmpp:jingle:transports:ibb:1' block-size='128'/>".parse().unwrap();
         let error = Transport::try_from(&elem).unwrap_err();
@@ -104,7 +100,7 @@ mod tests {
             Error::ParseError(string) => string,
             _ => panic!(),
         };
-        assert_eq!(message, "Required attribute 'sid' missing in JingleIBB element.");
+        assert_eq!(message, "Required attribute 'sid' missing.");
     }
 
     #[test]

src/lib.rs

@@ -23,6 +23,9 @@ extern crate sha3;
 extern crate blake2;
 
 macro_rules! get_attr {
+    ($elem:ident, $attr:tt, $type:tt) => (
+        get_attr!($elem, $attr, $type, value, value.parse()?)
+    );
     ($elem:ident, $attr:tt, optional, $value:ident, $func:expr) => (
         match $elem.attr($attr) {
             Some($value) => Some($func),

src/rsm.rs

@@ -61,7 +61,7 @@ impl<'a> TryFrom<&'a Element> for Set {
                 if set.first.is_some() {
                     return Err(Error::ParseError("Set can’t have more than one first."));
                 }
-                set.first_index = get_attr!(child, "index", optional, index, index.parse()?);
+                set.first_index = get_attr!(child, "index", optional);
                 set.first = Some(child.text());
             } else if child.is("index", ns::RSM) {
                 if set.index.is_some() {