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

Feat/default namespaces #45

Merged

Conversation

carlmontanari
Copy link
Contributor

👋 hey there, thanks for creating this project!

ive been slowly working on building a netconf client in zig and am using zig-xml to parse responses from servers as well as craft messages to send to them. unless ive missed something (highly possible!) it seems that currently there is no way to set a non-prefixed namespace (or to set a default namespace basically) on an element.

im sure some netconf servers behave better than others but at the very least the ones ive been testing with seem to be quite upset at me when prefixing the base netconf namespace. for example, sending the output from something like this:

    try writer.xmlDeclaration("UTF-8", null);
    try writer.elementStart("rpc");
    try writer.bindNs("nc", "urn:ietf:params:xml:ns:netconf:base:1.0");
    try writer.attribute("message-id", "101");
    try writer.elementStart("get-config");
    try writer.elementStart("source");
    try writer.elementStart("running");
    try writer.elementEnd();
    try writer.elementEnd();
    try writer.elementEnd();
    try writer.elementEnd();
    try writer.eof();

this server seems quite upset! with the changes here I can do all the same as above put set the prefix to null like:

    try writer.bindNs(null, "urn:ietf:params:xml:ns:netconf:base:1.0");

and my server happily returns its config.

im not super savvy on what is or is not in the xml standards or if this is something you'd be willing to include, but was messing around to see if I could get it to work so figured id just chuck a pr your way to see if you were interested!

ive updated the bindNs test w/ an element setup like this, and the conformance tests still seem to pass (zig build test in the xmlconf directory seems happy still).

feel free to close or holler if you'd like to see any changes. thanks again!

carl

Copy link
Owner

@ianprime0509 ianprime0509 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! I think this is a good improvement, it's definitely an oversight on my part that this wasn't already supported. My only feedback is that, for consistency with the rest of the API, the prefix argument should not be nullable: the empty string is used in other places to represent "no prefix". For example:

try expectEqualStrings("https://example.com/default", reader.namespaceUri(""));

@carlmontanari
Copy link
Contributor Author

carlmontanari commented Feb 2, 2025

Sounds good to me, will update prolly tomorrow morning!

just kidding, had a sec so just updated and re-tested and looks good! that also made it simpler which is nice :D

Copy link
Owner

@ianprime0509 ianprime0509 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@ianprime0509 ianprime0509 merged commit 31cb573 into ianprime0509:main Feb 2, 2025
4 checks passed
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