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

Add get_properties_ns to Node and RoNode #128

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

anwaralameddin
Copy link
Contributor

The method get_properties_ns was added to Node and RoNode in line with this issue.

It returns a HashMap of attribute values, which is indexed by tuples of the attributes and their namespaces.

The code is similar to that of get_properties. However, get_properties used unnecessary vector allocation attr_names, which is removed in the PR.

The indexing mentioned above required implementing the traits Hash, PartialEq and Eq for Namespace. These were implemented using the methods get_prefix and get_href instead of ns_ptr. This choice does not affect the method get_properties_ns as a prefix is bound to a single namespace name in a given scope. However, this has the effect that the two namespaces declared below are considered identical.

<root>
    <child1 xmlns:foo="http://www.example.com/myns" attribute="value1" foo:attribute="foo1" />
    <child2 xmlns:foo="http://www.example.com/myns" attribute="value2" foo:attribute="foo2" />
</root>

This required implementing the traits Hash, PartialEq and Eq for Namespace
Copy link
Member

@dginev dginev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, and the new tests!

Everything looks great, but I am wondering if the equality implementation would cause confusion down the road. Not opposed to merging, but wondering if you have additional motivation to share.

fn eq(&self, other: &Self) -> bool {
self.get_prefix() == other.get_prefix() && self.get_href() == other.get_href()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little tricky, since it can be surprising depending on what expectation someone approaches the Namespace struct.

Internally, Namespace stores a libxml2 xmlNsPtr. So one could have expected pointer equality. Alternatively, one could have expected the equality to be in the XML sense - if the URL is identical, then the namespaces are equal, even if the prefixes are different.

So introducing the "deepest" kind of equality here (same prefix and same namespace) isn't immediately expected behavior. Additionally, it is the slowest variant performance-wise, and uses dedicated allocations.

Hm... I am not opposed to merging it as introduced here, but I wonder if we'll get requests to modify it down the road. For now a clarifying comment on the Namespace struct could help.

self.get_prefix().hash(state);
self.get_href().hash(state);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to above, using the pointer directly could be another reasonable approach to hashing.

@anwaralameddin
Copy link
Contributor Author

Thank you for your review and comments!

I actually had conflicting thoughts about the three options. As you say, it depends on what one expects from the Namespace structure.

I would've liked it to be true to the specification, but I rolled out the URI comparison because I don't see it as an implementation of the XML namespace. I think of it as a NamespaceBinding structure, binding prefixes to namespace names, which is one reason for the current choice. Also, I wanted to preserve the (parts of) documents that are parsed and written out without further processing and didn't want this to potentially change prefixes.

Using the pointer felt unnecessarily too strict, given the usage of namespaces, but I don't have a strong argument against it. If you wish, we can change it to the strictest equality (using the xmlNsPtr), and those who desire a more relaxed notion can group the results as needed.

@dginev dginev merged commit 2380450 into KWARC:master Feb 25, 2024
1 check passed
@anwaralameddin anwaralameddin deleted the get_properties_ns branch February 25, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants