Added XmlWriter and Element::create API #2
Conversation
| /// Creates a new Element from the given name and attributes. | ||
| /// attributes are represented as an iterator over (key, value) tuples where | ||
| /// key and value are both &str. | ||
| pub fn create<'a, I: Iterator<Item = (&'a str, &'a str)>>(name: &str, attributes: I) -> Element { |
There was a problem hiding this comment.
Few remarks:
- you can use
newinstead ofcreateand rename the existing (private)newfunction intofrom_bufferfor instance (opened for other suggestion) (&'a str, &'a str)is not symmetric withAttributes::Item = (&[u8], &str). I wanted to avoid utf8 conversion when possible because most of the time we (I ?) compare it with statically known names, thus I could always usekey == b"mykey". Having symmetric behavior could help creating Elements based on other Elements (in your test, you had to usefrom_utf8), I think it is a common scenario- in your example, we could use
Vec<u8>and push bytes directly - an alternative is to have 2
Attributes-like iterators, one pure&[u8], another one&str
- in your example, we could use
|
That's awesome! |
|
Also might be good to update the readme 👍 |
Renamed Element::new to Element::from_buffer, and Element::create to Element::new. Reordered the functions for "new" to appear first, as seems to be the convention. Added an example in the documentation of how to use the XmlWriter and Element::new. Fixed the existing example by replacing println! by panic! in case of errors. Indeed the XML was erroneous, and the parser was emitting errors, but because we didn't panic, the test was passing. Fixed the XML to make the test pass.
|
Thanks for the review! I have made some changes to address these points: typos, comment on the write function, new instead of create. Rather than change the README (not testable), I have added an example in the lib.rs file, and also taken the liberty to improve the existing example and fix the XML. Ideally, the documentation could be hosted on Github pages from the output of rustdoc, to make sure the examples are always up to date and compile. Regarding the signature of Iterator<&str, &str> I know it's not symmetric, it just feels more natural in my opinion :-) I find it a bit low-level to do comparisons with byte string literals, but I understand that you want to avoid unnecessary UTF-8 conversion. In the case of the element's name, you have a choice: you can use either as_bytes() or as_str(). But for attributes, since the type is fixed on the Iterator implementation, it's a bit more difficult. At the same time, is that really a bottleneck on attributes? Generally, you match on element names first, so I assume this is where a program would typically spend most of its time checking. Alternatively, for the Element::new method, maybe I could use AsRef<[u8]> to support both (&[u8], &str) and (&str, &str); I haven't tried, but it seems doable. Then we would implement two iterator methods on Attributes to support both cases. Just let me know what your preference is :-) |
|
AsRef looks good to me ... Or Into perhaps
|
This uses AsRef<[u8]> which is implemented for [u8], str, and String
|
Works with AsRef, which I think is the way to go: Into would move its argument, while we only need to borrow it as a reference to a byte slice. In the future (in another contribution) I say we should try to improve Attributes to have the same flexibility as Element regarding the as_bytes()/as_str() duality. But for now it's good as it is I think :-) |
Added XmlWriter and Element::create API
|
Thanks a lot!! |
This PR adds the possibility to transform (add/change elements) and write XML. Two changes:
I added one test for the writer, and one test for the writer + element create. Also, note that I made the XmlWriter use a Write trait rather than BufWrite because it doesn't need its additional methods at the moment, but of course for best performance one should use a writer implementing BufWrite.