-
Notifications
You must be signed in to change notification settings - Fork 1
Fix XML namespace serialization to only add xmlns when needed #3
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,5 +12,92 @@ class XMLPydantifyModel(PydantifyModel): | |
| namespace: ClassVar[str] | ||
| prefix: ClassVar[str] | ||
|
|
||
| def model_dump_xml(self) -> etree.Element: | ||
| pass | ||
| def fields_to_elements( | ||
| self, | ||
| container_name: str | None = None, | ||
| parent_namespace: str | None = None, | ||
| ) -> list[etree._Element]: | ||
| """Convert model fields to XML elements. | ||
|
|
||
| Args: | ||
| container_name: If provided, wrap elements in a container with this name | ||
| parent_namespace: The namespace of the parent element, used to determine | ||
| if xmlns declaration is needed | ||
| """ | ||
| elements: list[etree._Element] = [] | ||
|
|
||
| # Get field info from model class (not instance to avoid deprecation warning) | ||
| for field_name, field_info in self.__class__.model_fields.items(): | ||
| value = getattr(self, field_name) | ||
| if value is None: | ||
| continue | ||
|
|
||
| # Get the XML element name from alias or field name | ||
| xml_name = field_info.alias if field_info.alias else field_name | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why the AI thinks it is a good idea to extract the namespace from the field alias rather than using the classvar. There is a reason the class vars exist, and the alias could be wrong. I don't see it as clean software engineering to magically extract information from a field that is not designed for it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay I see why it does it. In case the var name would be different to the field name. this could happen if the field name has a dash in it's name 🤔 |
||
| # Strip prefix (e.g., "configuration:devicename" -> "devicename") | ||
| if ":" in xml_name: | ||
| xml_name = xml_name.split(":", 1)[1] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not a fan of this at all |
||
|
|
||
| # Handle XMLPydantifyModel instances (nested models) | ||
| if isinstance(value, XMLPydantifyModel): | ||
| child_elements = value.fields_to_elements( | ||
| container_name=xml_name, | ||
| parent_namespace=self.namespace, | ||
| ) | ||
| elements.extend(child_elements) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Funny that it uses now the classvar |
||
| # Handle lists | ||
| elif isinstance(value, list): | ||
| for item in value: | ||
| if isinstance(item, XMLPydantifyModel): | ||
| child_elements = item.fields_to_elements( | ||
| container_name=xml_name, | ||
| parent_namespace=self.namespace, | ||
| ) | ||
| elements.extend(child_elements) | ||
| else: | ||
| # Primitive list item | ||
| elem = etree.Element(xml_name) | ||
| elem.text = str(item) | ||
| elements.append(elem) | ||
| else: | ||
| # Primitive field | ||
| elem = etree.Element(xml_name) | ||
| elem.text = str(value) | ||
| elements.append(elem) | ||
|
|
||
| # Wrap in container if container_name is provided | ||
| if container_name: | ||
| # Only add xmlns when namespace differs from parent | ||
| if parent_namespace is None or self.namespace != parent_namespace: | ||
| root = etree.Element(container_name, nsmap={None: self.namespace}) | ||
| else: | ||
| root = etree.Element(container_name) | ||
| for elem in elements: | ||
| root.append(elem) | ||
| return [root] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it works but could be done without nesting if's and for loop. The extra loop is not sexy |
||
|
|
||
| return elements | ||
|
|
||
| def model_dump_xml(self) -> etree._Element: | ||
| """Convert model to XML element tree.""" | ||
| # Get the first field and use it as the root element | ||
| for field_name, field_info in self.__class__.model_fields.items(): | ||
| value = getattr(self, field_name) | ||
| if value is None: | ||
| continue | ||
|
|
||
| xml_name = field_info.alias if field_info.alias else field_name | ||
| if ":" in xml_name: | ||
| xml_name = xml_name.split(":", 1)[1] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicated (ugly) code |
||
|
|
||
| if isinstance(value, XMLPydantifyModel): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really needed as the check is done in |
||
| elements = value.fields_to_elements( | ||
| container_name=xml_name, | ||
| parent_namespace=None, | ||
| ) | ||
| if elements: | ||
| return elements[0] | ||
|
|
||
| # Fallback: return self as root | ||
| elements = self.fields_to_elements(container_name=self.prefix) | ||
| return elements[0] if elements else etree.Element(self.prefix) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what the idea of the use of |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing model_fields is deprecated and also does not support excluding defaults or None. And not really a reason to access it over
__class__