Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dandyvica opened this issue Mar 14, 2017 · 5 comments
Open

Comments

@dandyvica
Copy link

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 ?

@netvl
Copy link
Owner

netvl commented Mar 15, 2017

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.

@dandyvica
Copy link
Author

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.

@netvl
Copy link
Owner

netvl commented Mar 16, 2017

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!

@dandyvica
Copy link
Author

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

@netvl netvl reopened this Mar 29, 2017
@netvl
Copy link
Owner

netvl commented Mar 29, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants