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

Fixed xml deserialization for missing prefixes #33

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

otto-ifak
Copy link
Contributor

The xml deserialization fails when xml tags without prefixes are used.

Copy link
Contributor

@jkhsjdhjs jkhsjdhjs left a comment

Choose a reason for hiding this comment

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

@otto-ifak I had a quick look at the change. Can you adjust it in accordance to the comments? Thanks!

@@ -85,8 +85,9 @@ def _tag_replace_namespace(tag: str, nsmap: Dict[str, str]) -> str:
"""
split = tag.split("}")
for prefix, namespace in nsmap.items():
if namespace == split[0][1:]:
return prefix + ":" + split[1]
if prefix:
Copy link
Contributor

Choose a reason for hiding this comment

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

Which value does prefix have when the prefix is missing? I would prefer an explicit check for that value (e.g. prefix is not None, if prefix were to be None when the prefix is missing).

basyx/aas/adapter/xml/xml_deserialization.py Outdated Show resolved Hide resolved
Comment on lines 88 to 89
if prefix:
if namespace == split[0][1:]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if prefix:
if namespace == split[0][1:]:
if prefix and namespace == split[0][1:]:

@s-heppner
Copy link
Contributor

I just realized that we don't have the right to edit this PR :D
@otto-ifak could you adapt those two changes please?

@otto-ifak
Copy link
Contributor Author

@s-heppner Done.

@s-heppner
Copy link
Contributor

Perfect, thank you!

@s-heppner s-heppner merged commit 6fd70b8 into eclipse-basyx:main Oct 3, 2023
5 checks passed
s-heppner pushed a commit that referenced this pull request Aug 16, 2024
* adapter.http: implement the attachment routes

* adapter.http: fix codestyle errors

* adapter.http: implement recommended changes

* adapter.http: implement new recommended changes
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.

3 participants