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

Retry XML parsing with huge_tree=True option. Fixes #185. #186

Closed
wants to merge 1 commit into from

Conversation

stacywsmith
Copy link
Contributor

Upon receiving an etree.XMLSyntaxError exception with a message containing huge text node retry the XML parsing using the huge_tree=True option.

@stacywsmith
Copy link
Contributor Author

This is one possible way of fixing #185. I only retry the parsing with the huge_tree=True option as needed. However, I'm not sure this is the best option. I'm definitely open to discussion on the correct fix.

@coveralls
Copy link

coveralls commented Apr 17, 2017

Coverage Status

Coverage decreased (-0.5%) to 69.57% when pulling 65c6838 on stacywsmith:issue_185 into 6d781dd on ncclient:master.

@stacywsmith
Copy link
Contributor Author

@leopoul Before I add unit tests to cover my changes, I'd appreciate your feedback on my approach.

I see three potential ways to address this:

  1. Always use the huge_tree=True option.
  2. Fall back to the huge_tree=True automatically if parsing fails due to a huge text node.
    (This is the option I implemented in this pull request.)
  3. Add a parameter to the manager to allow the user to request huge_tree support.

Thoughts?

@stacywsmith
Copy link
Contributor Author

I discussed this with a few folks, and I'm now thinking that option 3 in the above comment is a better approach. I'll close this pull request and submit a new one that uses option 3 to fix the issue.

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