INGK-1192-add-xcf-table-element #672
Conversation
polfeliu
changed the base branch from
master
to
INGK-1193-create-table-object-api
There was a problem hiding this comment.
Pull request overview
This PR adds support for table elements in XCF (configuration) files by introducing new classes (TableElement and ConfigTable) and extending the ConfigRegister class to support optional data attributes. The changes also refactor attribute extraction to use a centralized helper function with improved error messages.
Key Changes:
- Added
TableElementandConfigTableclasses for handling table data in configuration files - Extended
ConfigRegisterto support optional binary data attributes - Introduced a global
_get_attribute_from_elementhelper function with overloaded type signatures for better error handling - Updated XCF file format minor version from 1 to 2
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| ingenialink/configuration_file.py | Added TableElement and ConfigTable classes, extended ConfigRegister with data attribute support, introduced centralized attribute extraction helper, and updated XCF format to support tables |
| tests/test_configuration_file.py | Added comprehensive tests for TableElement and ConfigTable serialization/deserialization, updated ConfigRegister error message tests, and added tests for data attribute handling |
| agents.md | Added new documentation file for agent-related notes and styling preferences |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, device: Device) -> None: | ||
| self.major_version = self._SUPPORTED_MAJOR_VERSION | ||
| self.minor_version = 1 | ||
| self.minor_version = 2 |
There was a problem hiding this comment.
The minor version is being incremented from 1 to 2, but this appears to be a significant change that adds table support to the XCF format. Consider whether this should be a major version increment instead, or ensure the format remains backward compatible. If the format is designed to be backward compatible (i.e., older readers can safely ignore the Tables element), document this explicitly in the code or changelog.
| ) -> Optional[str]: ... | ||
|
|
||
|
|
||
| def _get_attribute_from_element( |
There was a problem hiding this comment.
separated from dict class to have easier access from other classes
Description
Added xcf table element to the xcf parser and associated methods
Type of change
Tests
Please describe the manual tests that you ran to verify your changes if it applies.
Documentation
Please update the documentation.
[Unreleased]section of the CHANGELOG.