Visitar URL original
Skip XML comments inside <node> and raise the passed error in handleError by zzoraemon043 · Pull Request #166 · pycollada/pycollada · GitHub
Skip to content

Skip XML comments inside <node> and raise the passed error in handleError - #166

Merged
jterrace merged 2 commits into
pycollada:masterfrom
zzoraemon043:fix/node-comments
Oct 19, 2026
Merged

jterrace merged 2 commits into
pycollada:masterfrom
zzoraemon043:fix/node-comments

Conversation

@zzoraemon043

@zzoraemon043 zzoraemon043 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #161

With lxml, Node.load got XML comments as children whose tag is a function, so a comment was reported as an unknown scene node. The lxml parser in createElementTree now uses remove_comments=True and remove_pis=True, which covers every element, not only <node>. The stdlib parser already drops them.

That error then surfaced as RuntimeError: No active exception to reraise, because handleError used a bare raise while it is often called outside an except block. It now raises the error it was given.

Added test_collada_with_comments, which loads a document with a comment and a processing instruction inside <node>. With lxml it fails before the change; pytest collada/tests passes with and without lxml (60 tests).

@jterrace jterrace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm curious - does the test you added already pass when lxml is not available? I think the default etree implementation ignores comments but lxml doesn't.

This only fixes comments in a single spot but it could happen elsewhere. How about updating here:
https://github.com/pycollada/pycollada/blob/master/collada/xmlutil.py#L121

to add remove_comments and remove_pis? See https://lxml.de/apidoc/lxml.etree.html#lxml.etree.XMLParser

@zzoraemon043

Copy link
Copy Markdown
Contributor Author

Good catch, the old test built the comment by hand so it failed without lxml too, but a parsed file never has comments there with the stdlib parser

Moved it to the parser in 72cb8aa with remove_comments and remove_pis and dropped the check in scene.py. The new test loads a whole document through Collada(), fails with lxml before the change and passes with and without lxml

@jterrace

Copy link
Copy Markdown
Member

thanks!

@jterrace
jterrace merged commit c9149a0 into pycollada:master Oct 19, 2026
1 check 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.

Error in Node.load when node contains an inline comment

2 participants