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 no_ns variants for Node and RoNode methods #129

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

anwaralameddin
Copy link
Contributor

This completes what started in another PR by adding the node methods has_property_no_ns, remove_property_no_ns and get_property_node_no_ns. These methods target nonprefixed properties and are needed when an element has multiple properties with the same local name, as in the example below:

<foo xmlns:ns="http://example.com" ns:bar="NS BAR" bar="BAR" />

Also, the method get_property_node_ns was added.

There was no added functionality to be gained by introducing set_property_no_ns as the existing method set_property sets the value for an attribute with no namespace.

While this adds functionality, it seems too verbose and repetitive. More ideally, one should have two variants of each method. One only takes an attribute name and does not consider namespaces:

pub fn foo_property(&self, name: &str)

The other takes an attribute name and, optionally, a namespace name:

pub fn foo_property_ns(&self, name: &str, ns: Option<&str>)

However, this will introduce braking changes for has_property_ns, get_property_ns and remove_property_ns. Could this be considered now?

Add has_property_no_ns, remove_property_no_ns and get_property_node_no_ns

Add get_property_node_ns
@dginev
Copy link
Member

dginev commented Feb 26, 2024

XML namespaces are notorious for causing API pain. There isn't a perfect solution for the current design of XML, as far as I'm concerned.

The situation you are guarding for should be an exceptionally rare one - and these verbose repetitive methods are the price one pays for wading into a design that shares attribute names in differing namespaces AND keeping namespace-free variants around.

The simple methods address the 99% of use cases that have unique local names cross-namespace, and I don't think we should be breaking them. If anything, I would personally be discouraging people that have nodes with foo="val" ns1:foo="val1" ns2:foo="val2", although I am certain one can motivate using these (such as simplifying XSLT for different targets by focusing on a different namespace for each target format).

I recall Perl has addressed this by adding a runtime cost where the namespace is checked in each getter/setter. But I would prefer redundancy to runtime penalty in Rust.

@anwaralameddin anwaralameddin marked this pull request as ready for review March 3, 2024 22:26
@dginev dginev merged commit 0dcce40 into KWARC:master Mar 12, 2024
1 check passed
@dginev
Copy link
Member

dginev commented Mar 12, 2024

Thanks again!

@anwaralameddin anwaralameddin deleted the property_no_ns branch March 12, 2024 16:39
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