KWARC / rust-libxml

Rust wrapper for libxml2
https://crates.io/crates/libxml
MIT License
76 stars 38 forks source link

Document::get_root_element #29

Closed triptec closed 6 years ago

triptec commented 6 years ago

So I've started working on pointer book keeping. The method below returns a Node with the doc_ptr as node_ptr if there is no root element. Is there a reason for this? I think this will add complexity as I would have to add it to a list of known nodes and do a lot of checks if the node is infact a node or a document. Would this be something we can change? I know we would like to keep api compatbility. https://github.com/KWARC/rust-libxml/blob/75f06557c274581cbd60a01b25d9587bb620febb/src/tree.rs#L100

dginev commented 6 years ago

I wanted to mirror the simplicity of the $root = $dom->documentElement(); in Perl's XML::LibXML when I wrote that, and using the document pointer was some stopgap to avoid returning a "None" if there was no root.

However, I am now seeing the Perl API is in fact more honest and returns an undef when there is no element: https://github.com/shlomif/perl-XML-LibXML/blob/master/LibXML.xs#L3720

So I agree with you that a refactor is needed here. I don't mind changing the return type at all, as long as we honestly reflect that in the release numbers, which we will.

dginev commented 6 years ago

I take it you were suggesting an Option<Node> return type instead.

triptec commented 6 years ago

Yes, if there wasn't something that I hadn't thought about. Good

dginev commented 6 years ago

Closing here, looking forward to the refactor branch :+1: Quite excited to "enlighten" the wrapper with your RefCell approach