Use AttrError in the Attributes iterator return type

Because now error type is PartialEq, we can use `assert_eq!` directly, which is especially nice when used with pretty_assertions crate
Mingun · May 3, 2022 · da9e110 · da9e110
1 parent 30edbfe
commit da9e110
Show file tree

Hide file tree

Showing 8 changed files with 149 additions and 132 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -36,6 +36,7 @@
 - fix: do not unescape CDATA content because it never escaped by design.
   CDATA event data now represented by its own `BytesCData` type
   ([quick-xml#311](https://github.com/tafia/quick-xml/issues/311))
+- refactor: now `Attributes::next()` returns a new type `AttrError` when attribute parsing failed
 
 ## 0.23.0-alpha3
 

diff --git a/src/errors.rs b/src/errors.rs
@@ -219,4 +219,11 @@ pub mod serialize {
             DeError::Float(e)
         }
     }
+
+    impl From<AttrError> for DeError {
+        #[inline]
+        fn from(e: AttrError) -> Self {
+            DeError::Xml(e.into())
+        }
+    }
 }
diff --git a/src/events/attributes.rs b/src/events/attributes.rs
@@ -2,7 +2,7 @@
 //!
 //! Provides an iterator over attributes key/value pairs
 
-use crate::errors::{Error, Result};
+use crate::errors::{Error, Result as XmlResult};
 use crate::escape::{do_unescape, escape};
 use crate::reader::{is_whitespace, Reader};
 use std::fmt::{Debug, Display, Formatter};
@@ -37,7 +37,7 @@ impl<'a> Attribute<'a> {
     /// This will allocate if the value contains any escape sequences.
     ///
     /// See also [`unescaped_value_with_custom_entities()`](#method.unescaped_value_with_custom_entities)
-    pub fn unescaped_value(&self) -> Result<Cow<[u8]>> {
+    pub fn unescaped_value(&self) -> XmlResult<Cow<[u8]>> {
         self.make_unescaped_value(None)
     }
 
@@ -57,14 +57,14 @@ impl<'a> Attribute<'a> {
     pub fn unescaped_value_with_custom_entities(
         &self,
         custom_entities: &HashMap<Vec<u8>, Vec<u8>>,
-    ) -> Result<Cow<[u8]>> {
+    ) -> XmlResult<Cow<[u8]>> {
         self.make_unescaped_value(Some(custom_entities))
     }
 
     fn make_unescaped_value(
         &self,
         custom_entities: Option<&HashMap<Vec<u8>, Vec<u8>>>,
-    ) -> Result<Cow<[u8]>> {
+    ) -> XmlResult<Cow<[u8]>> {
         do_unescape(&*self.value, custom_entities).map_err(Error::EscapeError)
     }
 
@@ -78,7 +78,7 @@ impl<'a> Attribute<'a> {
     ///
     /// [`unescaped_value()`]: #method.unescaped_value
     /// [`Reader::decode()`]: ../../reader/struct.Reader.html#method.decode
-    pub fn unescape_and_decode_value<B: BufRead>(&self, reader: &Reader<B>) -> Result<String> {
+    pub fn unescape_and_decode_value<B: BufRead>(&self, reader: &Reader<B>) -> XmlResult<String> {
         self.do_unescape_and_decode_value(reader, None)
     }
 
@@ -100,7 +100,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &Reader<B>,
         custom_entities: &HashMap<Vec<u8>, Vec<u8>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         self.do_unescape_and_decode_value(reader, Some(custom_entities))
     }
 
@@ -110,7 +110,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &Reader<B>,
         custom_entities: Option<&HashMap<Vec<u8>, Vec<u8>>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         let decoded = reader.decode(&*self.value);
         let unescaped =
             do_unescape(decoded.as_bytes(), custom_entities).map_err(Error::EscapeError)?;
@@ -122,7 +122,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &Reader<B>,
         custom_entities: Option<&HashMap<Vec<u8>, Vec<u8>>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         let decoded = reader.decode(&*self.value)?;
         let unescaped =
             do_unescape(decoded.as_bytes(), custom_entities).map_err(Error::EscapeError)?;
@@ -140,7 +140,7 @@ impl<'a> Attribute<'a> {
     pub fn unescape_and_decode_without_bom<B: BufRead>(
         &self,
         reader: &mut Reader<B>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         self.do_unescape_and_decode_without_bom(reader, None)
     }
 
@@ -155,7 +155,7 @@ impl<'a> Attribute<'a> {
     pub fn unescape_and_decode_without_bom<B: BufRead>(
         &self,
         reader: &Reader<B>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         self.do_unescape_and_decode_without_bom(reader, None)
     }
 
@@ -175,7 +175,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &mut Reader<B>,
         custom_entities: &HashMap<Vec<u8>, Vec<u8>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         self.do_unescape_and_decode_without_bom(reader, Some(custom_entities))
     }
 
@@ -195,7 +195,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &Reader<B>,
         custom_entities: &HashMap<Vec<u8>, Vec<u8>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         self.do_unescape_and_decode_without_bom(reader, Some(custom_entities))
     }
 
@@ -204,7 +204,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &mut Reader<B>,
         custom_entities: Option<&HashMap<Vec<u8>, Vec<u8>>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         let decoded = reader.decode_without_bom(&*self.value);
         let unescaped =
             do_unescape(decoded.as_bytes(), custom_entities).map_err(Error::EscapeError)?;
@@ -216,7 +216,7 @@ impl<'a> Attribute<'a> {
         &self,
         reader: &Reader<B>,
         custom_entities: Option<&HashMap<Vec<u8>, Vec<u8>>>,
-    ) -> Result<String> {
+    ) -> XmlResult<String> {
         let decoded = reader.decode_without_bom(&*self.value)?;
         let unescaped =
             do_unescape(decoded.as_bytes(), custom_entities).map_err(Error::EscapeError)?;
@@ -337,7 +337,8 @@ impl<'a> Attributes<'a> {
 }
 
 impl<'a> Iterator for Attributes<'a> {
-    type Item = Result<Attribute<'a>>;
+    type Item = Result<Attribute<'a>, AttrError>;
+
     fn next(&mut self) -> Option<Self::Item> {
         let len = self.bytes.len();
 

diff --git a/src/events/mod.rs b/src/events/mod.rs
@@ -402,7 +402,7 @@ impl<'a> BytesDecl<'a> {
                 Err(Error::XmlDeclWithoutVersion(Some(found)))
             }
             // error parsing attributes
-            Some(Err(e)) => Err(e),
+            Some(Err(e)) => Err(e.into()),
             // no attributes
             None => Err(Error::XmlDeclWithoutVersion(None)),
         }

diff --git a/tests/documents/html5.txt b/tests/documents/html5.txt
@@ -1,7 +1,7 @@
 DocType(html)
 Characters(
 )
-StartElement(a, attr-error: error while parsing attribute: position 7: attribute value must be enclosed in `"` or `'`)
+StartElement(a, attr-error: position 7: attribute value must be enclosed in `"` or `'`)
 Characters(Hey)
 EndElement(a)
 Characters(

diff --git a/tests/test.rs b/tests/test.rs
@@ -32,25 +32,22 @@ fn test_attributes_empty() {
     let mut buf = Vec::new();
     match r.read_event(&mut buf) {
         Ok(Empty(e)) => {
-            let mut atts = e.attributes();
-            match atts.next() {
+            let mut attrs = e.attributes();
+            assert_eq!(
+                attrs.next(),
                 Some(Ok(Attribute {
                     key: b"att1",
                     value: Cow::Borrowed(b"a"),
-                })) => (),
-                e => panic!("Expecting att1='a' attribute, found {:?}", e),
-            }
-            match atts.next() {
+                }))
+            );
+            assert_eq!(
+                attrs.next(),
                 Some(Ok(Attribute {
                     key: b"att2",
                     value: Cow::Borrowed(b"b"),
-                })) => (),
-                e => panic!("Expecting att2='b' attribute, found {:?}", e),
-            }
-            match atts.next() {
-                None => (),
-                e => panic!("Expecting None, found {:?}", e),
-            }
+                }))
+            );
+            assert_eq!(attrs.next(), None);
         }
         e => panic!("Expecting Empty event, got {:?}", e),
     }
@@ -64,18 +61,15 @@ fn test_attribute_equal() {
     let mut buf = Vec::new();
     match r.read_event(&mut buf) {
         Ok(Empty(e)) => {
-            let mut atts = e.attributes();
-            match atts.next() {
+            let mut attrs = e.attributes();
+            assert_eq!(
+                attrs.next(),
                 Some(Ok(Attribute {
                     key: b"att1",
                     value: Cow::Borrowed(b"a=b"),
-                })) => (),
-                e => panic!("Expecting att1=\"a=b\" attribute, found {:?}", e),
-            }
-            match atts.next() {
-                None => (),
-                e => panic!("Expecting None, found {:?}", e),
-            }
+                }))
+            );
+            assert_eq!(attrs.next(), None);
         }
         e => panic!("Expecting Empty event, got {:?}", e),
     }
@@ -116,7 +110,7 @@ fn test_attributes_empty_ns() {
         e => panic!("Expecting Empty event, got {:?}", e),
     };
 
-    let mut atts = e
+    let mut attrs = e
         .attributes()
         .map(|ar| ar.expect("Expecting attribute parsing to succeed."))
         // we don't care about xmlns attributes for this test
@@ -125,23 +119,19 @@ fn test_attributes_empty_ns() {
             let (opt_ns, local_name) = r.attribute_namespace(name, &ns_buf);
             (opt_ns, local_name, value)
         });
-    match atts.next() {
-        Some((None, b"att1", Cow::Borrowed(b"a"))) => (),
-        e => panic!("Expecting att1='a' attribute, found {:?}", e),
-    }
-    match atts.next() {
-        Some((Some(ns), b"att2", Cow::Borrowed(b"b"))) => {
-            assert_eq!(&ns[..], b"urn:example:r");
-        }
-        e => panic!(
-            "Expecting {{urn:example:r}}att2='b' attribute, found {:?}",
-            e
-        ),
-    }
-    match atts.next() {
-        None => (),
-        e => panic!("Expecting None, found {:?}", e),
-    }
+    assert_eq!(
+        attrs.next(),
+        Some((None, &b"att1"[..], Cow::Borrowed(&b"a"[..])))
+    );
+    assert_eq!(
+        attrs.next(),
+        Some((
+            Some(&b"urn:example:r"[..]),
+            &b"att2"[..],
+            Cow::Borrowed(&b"b"[..])
+        ))
+    );
+    assert_eq!(attrs.next(), None);
 }
 
 /// Single empty element with qualified attributes.
@@ -161,7 +151,7 @@ fn test_attributes_empty_ns_expanded() {
             e => panic!("Expecting Empty event, got {:?}", e),
         };
 
-        let mut atts = e
+        let mut attrs = e
             .attributes()
             .map(|ar| ar.expect("Expecting attribute parsing to succeed."))
             // we don't care about xmlns attributes for this test
@@ -170,23 +160,19 @@ fn test_attributes_empty_ns_expanded() {
                 let (opt_ns, local_name) = r.attribute_namespace(name, &ns_buf);
                 (opt_ns, local_name, value)
             });
-        match atts.next() {
-            Some((None, b"att1", Cow::Borrowed(b"a"))) => (),
-            e => panic!("Expecting att1='a' attribute, found {:?}", e),
-        }
-        match atts.next() {
-            Some((Some(ns), b"att2", Cow::Borrowed(b"b"))) => {
-                assert_eq!(&ns[..], b"urn:example:r");
-            }
-            e => panic!(
-                "Expecting {{urn:example:r}}att2='b' attribute, found {:?}",
-                e
-            ),
-        }
-        match atts.next() {
-            None => (),
-            e => panic!("Expecting None, found {:?}", e),
-        }
+        assert_eq!(
+            attrs.next(),
+            Some((None, &b"att1"[..], Cow::Borrowed(&b"a"[..])))
+        );
+        assert_eq!(
+            attrs.next(),
+            Some((
+                Some(&b"urn:example:r"[..]),
+                &b"att2"[..],
+                Cow::Borrowed(&b"b"[..])
+            ))
+        );
+        assert_eq!(attrs.next(), None);
     }
 
     match r.read_namespaced_event(&mut buf, &mut ns_buf) {
@@ -226,7 +212,7 @@ fn test_default_ns_shadowing_empty() {
             e => panic!("Expecting Empty event, got {:?}", e),
         };
 
-        let mut atts = e
+        let mut attrs = e
             .attributes()
             .map(|ar| ar.expect("Expecting attribute parsing to succeed."))
             // we don't care about xmlns attributes for this test
@@ -237,14 +223,11 @@ fn test_default_ns_shadowing_empty() {
             });
         // the attribute should _not_ have a namespace name. The default namespace does not
         // apply to attributes.
-        match atts.next() {
-            Some((None, b"att1", Cow::Borrowed(b"a"))) => (),
-            e => panic!("Expecting att1='a' attribute, found {:?}", e),
-        }
-        match atts.next() {
-            None => (),
-            e => panic!("Expecting None, found {:?}", e),
-        }
+        assert_eq!(
+            attrs.next(),
+            Some((None, &b"att1"[..], Cow::Borrowed(&b"a"[..])))
+        );
+        assert_eq!(attrs.next(), None);
     }
 
     // </outer>
@@ -288,7 +271,7 @@ fn test_default_ns_shadowing_expanded() {
             }
             e => panic!("Expecting Start event (<inner>), got {:?}", e),
         };
-        let mut atts = e
+        let mut attrs = e
             .attributes()
             .map(|ar| ar.expect("Expecting attribute parsing to succeed."))
             // we don't care about xmlns attributes for this test
@@ -299,14 +282,11 @@ fn test_default_ns_shadowing_expanded() {
             });
         // the attribute should _not_ have a namespace name. The default namespace does not
         // apply to attributes.
-        match atts.next() {
-            Some((None, b"att1", Cow::Borrowed(b"a"))) => (),
-            e => panic!("Expecting att1='a' attribute, found {:?}", e),
-        }
-        match atts.next() {
-            None => (),
-            e => panic!("Expecting None, found {:?}", e),
-        }
+        assert_eq!(
+            attrs.next(),
+            Some((None, &b"att1"[..], Cow::Borrowed(&b"a"[..])))
+        );
+        assert_eq!(attrs.next(), None);
     }
 
     // virtual </inner>