Skip to content

Support Vantor XML namespaces#23

Draft
klassenjs wants to merge 3 commits into
masterfrom
vantor-xml-namespaces
Draft

Support Vantor XML namespaces#23
klassenjs wants to merge 3 commits into
masterfrom
vantor-xml-namespaces

Conversation

@klassenjs

Copy link
Copy Markdown
Contributor

No description provided.

James S Klassen added 2 commits June 13, 2026 20:50
@klassenjs klassenjs marked this pull request as ready for review June 15, 2026 15:02
@klassenjs klassenjs requested a review from clairecporter June 15, 2026 15:02

@klassenjs klassenjs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this would use a proper XML parser, but I'm sticking with keeping it as a close port of the Matlab source for now.

But that does raise the question of if we need to also fix the Matlab code?

Comment thread lib/filter_scene.py
for ps in xml_paramstrs_left:
if ps in line:
values[xml_paramstrs.index(ps)] = line.replace("<{}>".format(ps), '').replace("</{}>".format(ps), '')
regex = "<([^:]*:){0,1}"+ps+">(.*)</([^:]*:){0,1}"+ps+">"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically, this should validate that ps doesn't have anything funky in it that could be interpreted as part of the regex instead of just a literal. In practice, ps is controlled by trusted code so it would be unlikely for it to contain something unexpected.

And is not going to cause regex injection issues.  This isn't
a problem given the current call tree, but this is good defensive coding.
@klassenjs klassenjs marked this pull request as draft June 16, 2026 15:30
@klassenjs

Copy link
Copy Markdown
Contributor Author

Back to draft status until this change actually runs successfully.

@clairecporter

Copy link
Copy Markdown
Member

@klassenjs The Matlab version is long deprecated. No need to update it there.

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