xml-rs icon indicating copy to clipboard operation
xml-rs copied to clipboard

Allow to access a HashMap of OwnAttributes instead of Vec<OwnedAttribute>

Open dandyvica opened this issue 7 years ago • 5 comments

Hi,

In lots of languages (Python, Ruby,...), it's easy to query an xml element attribute by giving its name, using a dict (in Python) or a hash (in Ruby) to hold all element attributes.

Why not using a

HashMap<String, OwnedAttribute>

here ?

dandyvica avatar Mar 14 '17 23:03 dandyvica

That's because I wanted to make reading and writing events as close as possible, and for reading events to be cheaply convertible to writing events. Also, doing exactly what you have written is not possible, because the name of an attribute is OwnedName, which is a qualified XML name, not just String, therefore you will have to construct a full-fledged OwnedName just to perform a lookup in such table, which is inconvenient and has performance impact.

From the performance point of view of the collections themselves, I sincerely doubt that it really matters whether it is a linear Vec or a HashMap, even when you want to look for attributes by their names. Unless there are hundreds of thousands attributes, and unless you need to perform hundreds of thousands lookups on them per second (which, I believe, is quite a rare problem), performance won't matter much; moreover, I expect that in the case when the number of attributes is in single digits, which I believe is the most common case, Vec should perform better than HashMap. And remember that you'll have to construct an OwnedName for each query, which would require at least one allocation.

As for syntax for accessing elements by the name, using iterator methods doesn't seem to be a great detriment to me:

if let Some(attr) = attributes.iter().find(|a| a.name.local_name == "something") {
  ..
}

This also allows querying by the namespace, when it is important, using the same syntax.

netvl avatar Mar 15 '17 09:03 netvl

Hi netvl,

I was not referring to any perf issue on using Vec or HashMap, but rather to the ease of development. For me, it seems rather natural to refer to an attribute by its name.

But as you said, the developer can add some helper fn to achieve the same result. I was surprised to not get the same level of smoothness as in Python or Ruby.

dandyvica avatar Mar 15 '17 22:03 dandyvica

For me, it seems rather natural to refer to an attribute by its name.

As I said, the problem is that attributes cannot be named by strings, they have to be named by the structural names, because attribute names are qualified. Therefore, there just cannot be a HashMap<String, _>, there have to be HashMap<OwnedName, _>, and suddenly you have to construct an entire owned name to query the map:

StartElement { attributes, .. } => {
    if let Some(attr) = attributes.get(&Name::local("something").to_owned()) {
        ...
    }
}

which not only is longer, but also requires an allocation for each access.

That being said, I probably could write a wrapper around Vec<OwnedAttribute>, which would provide map-like accessors to attributes, something like this:

struct OwnedAttributes(Vec<OwnedAttribute>);

impl OwnedAttributes {
    fn get<'a>(&'a self, name: Name) -> Option<&'a OwnedAttribute> {
        self.0.iter().find(|attr| attr.name.borrow() == name)
    }
}

which then could be used as

StartElement { attributes, .. } => {
    if let Some(attr) = attributes.get(Name::local("whatever")) {
        ...
    }
}

This is probably a good idea, I may add something like this in future versions. Thanks!

netvl avatar Mar 16 '17 08:03 netvl

Yes, it will probably minimize some match arms. Thanks for taking this into account !

dandyvica avatar Mar 28 '17 18:03 dandyvica

I'll keep this open until the feature is implemented.

netvl avatar Mar 29 '17 11:03 netvl