Repository: tafia/quick-xml
Pull request #5 (2016-02-26 16:20:30)

improve Element API with new, push_attribute, extend_from_attributes
Implemented #3, now using IntoIterator so we don't need to call .into_iter() on collections, and permitted chained calls (the two additional methods return &mut Element)

tafia: I have mixed feelings for `extend_attributes` and `push_attribute` returning `&mut Element`. 
I understand that it's helpful to chain things together but at the same time it looks too different to what `Vec` is doing.
For chaining, I think what is expected is more a `with_attributes(self, iter)` which takes ownership of self in the meantime and returns it.

```rust
// chaining function ... initialization
impl Element {
    fn with_attributes(self, iter) -> Element;
    fn push_attribute(&mut self, att: (...));
    fn extend_attributes(&mut self, iter: ...);
}
```
matt2xu: The spurious tabs are killing me, I'm using Eclipse and despite being configured for spaces only it still inserts tabs in certain cases. What editor do you recommend for coding in Rust?

Agreed for from_utf8. :+1: for with_attributes consuming self, I like that. For push_attribute and extend_attributes the chaining is less useful, because in the end the element must be put in an Event, and that consumes it. So I'm OK with push_attribute not returning anything. If we have with_attributes and push_attribute, do we still need extend_attributes?
tafia: I used to write on Atom at home and vim at work. I'm fully vim now.

I agree we can skip `extend_attributes` for the moment.

Patch:
diff --git a/.gitignore b/.gitignore
index a9d37c5..f211bdd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,2 +1,3 @@
 target
 Cargo.lock
+.project
diff --git a/src/lib.rs b/src/lib.rs
index 8e8d365..a456416 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -48,14 +48,15 @@
 //!             // collect existing attributes
 //!             let mut attrs = e.attributes().map(|attr| attr.unwrap()).collect::<Vec<_>>();
 //!
-//!             // adds a new my-key="some value" attribute
-//!             attrs.push((b"my-key", "some value"));
+//!             // copy existing attributes, adds a new my-key="some value" attribute
+//!             let mut elem = Element::new("my_elem").with_attributes(attrs);
+//!             elem.push_attribute(b"my-key", "some value");
 //!
 //!             // writes the event to the writer
-//!             assert!(writer.write(Start(Element::new("my_elem", attrs.into_iter()))).is_ok());
+//!             assert!(writer.write(Start(elem)).is_ok());
 //!         },
 //!         Ok(Event::End(ref e)) if e.as_bytes() == b"this_tag" => {
-//!             assert!(writer.write(End(Element::new("my_elem", iter::empty::<(&str, &str)>()))).is_ok());
+//!             assert!(writer.write(End(Element::new("my_elem"))).is_ok());
 //!         },
 //!         Ok(e) => assert!(writer.write(e).is_ok()),
 //!         Err(e) => panic!("{:?}", e),
@@ -80,7 +81,7 @@ mod test;
 
 use std::fs::File;
 use std::io::{self, BufRead, BufReader, Write};
-use std::iter::Iterator;
+use std::iter::IntoIterator;
 use std::path::Path;
 use std::fmt;
 use std::str::from_utf8;
@@ -406,28 +407,16 @@ pub struct Element {
 
 impl Element {
 
-    /// Creates a new Element from the given name and attributes.
-    /// attributes are represented as an iterator over (key, value) tuples.
-    /// Key and value can be anything that implements the AsRef<[u8]> trait,
-    /// like byte slices and strings.
-    pub fn new<'a, K, V, I>(name: &str, attributes: I) -> Element 
-        where K: AsRef<[u8]>, V: AsRef<[u8]>, I: Iterator<Item = (K, V)>
-    {
-        let mut bytes = Vec::from(name.as_bytes());
-        let name_end = bytes.len();
-        for attr in attributes {
-            bytes.push(b' ');
-            bytes.extend_from_slice(attr.0.as_ref());
-            bytes.extend_from_slice(b"=\"");
-            bytes.extend_from_slice(attr.1.as_ref());
-            bytes.push(b'"');
-        }
+    /// Creates a new Element from the given name.
+    /// name is a reference that can be converted to a byte slice, such as &[u8] or &str
+    pub fn new<A>(name: A) -> Element where A: AsRef<[u8]> {
+        let bytes = Vec::from(name.as_ref());
         let end = bytes.len();
         Element {
             buf: bytes,
             start: 0,
             end: end,
-            name_end: name_end
+            name_end: end
         }
     }
 
@@ -441,6 +430,18 @@ impl Element {
         }
     }
 
+    /// Consumes self and adds attributes to this element from an iterator over (key, value) tuples.
+    /// Key and value can be anything that implements the AsRef<[u8]> trait,
+    /// like byte slices and strings.
+    pub fn with_attributes<K, V, I>(mut self, attributes: I) -> Self
+        where K: AsRef<[u8]>, V: AsRef<[u8]>, I: IntoIterator<Item = (K, V)>
+    {
+        for attr in attributes {
+            self.push_attribute(attr.0, attr.1);
+        }
+        self
+    }
+
     /// name as &[u8] (without eventual attributes)
     pub fn as_bytes(&self) -> &[u8] {
         &self.buf[self.start..self.name_end]
@@ -462,6 +463,21 @@ impl Element {
     pub fn into_string(self) -> Result<String> {
         ::std::string::String::from_utf8(self.buf).map_err(|e| Error::Utf8(e.utf8_error()))
     }
+
+    /// Adds an attribute to this element from the given key and value.
+    /// Key and value can be anything that implements the AsRef<[u8]> trait,
+    /// like byte slices and strings.
+    pub fn push_attribute<K, V>(&mut self, key: K, value: V)
+        where K: AsRef<[u8]>, V: AsRef<[u8]>
+    {
+        let bytes = &mut self.buf;
+        bytes.push(b' ');
+        bytes.extend_from_slice(key.as_ref());
+        bytes.extend_from_slice(b"=\"");
+        bytes.extend_from_slice(value.as_ref());
+        bytes.push(b'"');
+        self.end = bytes.len();
+    }
 }
 
 impl fmt::Debug for Element {


Test patch:
diff --git a/src/test.rs b/src/test.rs
index 0f730cf..23f5b23 100644
--- a/src/test.rs
+++ b/src/test.rs
@@ -141,22 +141,21 @@ fn test_writer() {
 
 #[test]
 fn test_write_attrs() {
-    let str_from = r#"<elem attr="val"></elem>"#;
-    let expected = r#"<elem attr="val" another="one"></elem>"#;
+    let str_from = r#"<source attr="val"></source>"#;
+    let expected = r#"<copy attr="val" a="b" c="d" x="y"></copy>"#;
     let reader = XmlReader::from_str(&str_from).trim_text(true);
     let mut writer = XmlWriter::new(Cursor::new(Vec::new()));
     for event in reader {
         let event = event.unwrap();
         let event = match event {
             Start(elem) => {
-                let mut attrs = elem.attributes().map(|e| {
-                    let (k,v) = e.unwrap();
-                    (from_utf8(k).unwrap(), v)
-                }).collect::<Vec<_>>();
-                attrs.push(("another", "one"));
-                let elem = Element::new("elem", attrs.into_iter());
+                let mut attrs = elem.attributes().map(|e| e.unwrap()).collect::<Vec<_>>();
+                attrs.extend_from_slice(&[(b"a", "b"), (b"c", "d")]);
+                let mut elem = Element::new("copy").with_attributes(attrs);
+                elem.push_attribute("x", "y");
                 Start(elem)
             },
+            End(_elem) => End(Element::new("copy")),
             _ => event
         };
         assert!(writer.write(event).is_ok());