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 child node performance

Merged
merged 2 commits into from Apr 26, 2017
Merged

Add child node performance #175

merged 2 commits into from Apr 26, 2017

Conversation

heckendorfc
Copy link
Contributor

@heckendorfc heckendorfc commented Apr 10, 2017

I generate XML documents which can easily have nodes with >10k children. With the current logic in this package, there is no way to add child nodes without iterating through or creating R objects containing the complete list of children.

I restructured xml_add_child so that using .where=0L (prepend mode) will bypass these very slow function calls.

With the existing code, using .where=0L will produce an error if there are no children, rather than just insert the node as the first and only child. I assume this is a bug so I also included a fix for this in my changes.

@jimhester
Copy link
Member

jimhester commented Apr 10, 2017

This looks good, thank you for the PR! Could you add a test for the .where = 0 case with no children to tests/testthat/test-modify-xml.R? Also please add a note to NEWS.md referencing this PR # and your GitHub handle. Thanks!

@heckendorfc
Copy link
Contributor Author

heckendorfc commented Apr 10, 2017

I can't reproduce that error I was talking about, so I'm going to guess I'm either thinking about something in XML rather than this package, or something I encountered during development. It's been a few days since I worked on this code.

The .where = 0L case should already be handled by the first test in:
"xml_add_child can insert anywhere in the child list"

Recent commit adds NOTES message and fixes the improper text node handling the tests picked up.

edit: oops, NEWS, not NOTES

@jimhester jimhester merged commit 3eb7e69 into r-lib:master Apr 26, 2017
3 of 4 checks passed
@jimhester
Copy link
Member

jimhester commented Apr 26, 2017

Thanks!

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.

None yet

2 participants