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

Document::get_root_element #29

Closed
triptec opened this issue Jan 26, 2018 · 4 comments
Closed

Document::get_root_element #29

triptec opened this issue Jan 26, 2018 · 4 comments

Comments

@triptec
Copy link
Collaborator

triptec commented Jan 26, 2018

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.

pub fn get_root_element(&self) -> Node {

@dginev
Copy link
Member

dginev commented Jan 26, 2018

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
Copy link
Member

dginev commented Jan 26, 2018

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

@triptec
Copy link
Collaborator Author

triptec commented Jan 26, 2018

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

@dginev
Copy link
Member

dginev commented Jan 31, 2018

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

@dginev dginev closed this as completed Jan 31, 2018
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

No branches or pull requests

2 participants