INGK-1194 parse tables from dictionaries #665
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements functionality to parse and manage table definitions from dictionary files (INGK-1194). The implementation adds support for reading table metadata from XML dictionaries, storing them in a structured format, and providing accessor methods to retrieve table information.
Key Changes
- Added
DictionaryTabledataclass to represent table metadata including table ID, axis, index register ID, and value register ID - Implemented
get_table()method to retrieve tables with support for axis-specific and multi-axis lookups - Added
_read_tables()parsing method for extracting table information from XML dictionary files - Integrated table merging support in dictionary addition operations
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ingenialink/dictionary.py |
Core implementation of table parsing, storage, and retrieval functionality with new DictionaryTable dataclass and related methods |
tests/test_dictionary.py |
Comprehensive test coverage for table parsing and retrieval including parameterized tests across multiple interfaces and edge case handling |
tests/resources/dictionary_with_tables_minimal.xdf3 |
New test fixture providing minimal XML dictionary with table definitions for all supported device types (ECAT, EoE, ETH, CAN) |
tests/resources/__init__.py |
Added constant for new test dictionary resource file |
pyproject.toml |
Updated summit-testing-framework dependency version |
poetry.lock |
Lock file updates reflecting dependency version change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.safety_modules = {} | ||
| self._registers = {} | ||
| self.subnodes = {} | ||
| self._tables = dict[int, dict[str, DictionaryTable]]() |
There was a problem hiding this comment.
The type hint syntax dict[int, dict[str, DictionaryTable]]() is incorrect. When initializing an empty dictionary, you should use {} without the type annotation, or use dict() without the type parameter. The correct syntax would be:
self._tables: dict[int, dict[str, DictionaryTable]] = {}or simply:
self._tables = {}The current syntax will raise a TypeError at runtime.
| self._tables = dict[int, dict[str, DictionaryTable]]() | |
| self._tables = {} |
There was a problem hiding this comment.
@copilot according to what rule, isn't dict a generic typevar?
| axis = int(_axis) if _axis is not None else 0 | ||
| id_index = str(table_element.attrib.get(self.__TABLE_ID_INDEX_ATTR)) | ||
| id_value = str(table_element.attrib.get(self.__TABLE_ID_VALUE_ATTR)) | ||
|
|
||
| table = DictionaryTable( | ||
| id=uid, | ||
| axis=axis if _axis is not None else None, |
There was a problem hiding this comment.
The logic for determining the axis value has a potential issue. When _axis is None, you set axis = 0 on line 1614, but then on line 1620 you check if _axis is not None to determine what to store in the DictionaryTable.axis field. This creates an inconsistency:
- For tables without an axis attribute in XML,
axiswill be0(integer) for dictionary storage - But
table.axiswill beNone
This means a table without an axis attribute will be stored in self._tables[0] but its axis property will be None. This could cause confusion. Consider either:
- Storing tables without axis in a different key (e.g.,
-1orNoneif the dict supports it) - Ensuring consistency by setting
table.axis = 0when no axis is specified
Looking at the test expectations, it seems tables without axis should be stored at index 0 but have axis=None, which is the current behavior. However, this should be clearly documented.
There was a problem hiding this comment.
Yes, this is a bit confusing, also for me. This just replicates the structure for registers, that for non-axis are also stored on axis 0. Also note that the index of _tables is something private to the dictionary.
table.axis is public
| with pytest.raises(ValueError, match=f"Table {uid} not found."): | ||
| dictionary.get_table(uid=uid, axis=None) | ||
| # Specify a uid and axis that does not exist | ||
| with pytest.raises(KeyError, match="axis=3 does not exist."): |
There was a problem hiding this comment.
[nitpick] The error message assertion on line 570 expects the literal string "axis=3 does not exist." but looking at the implementation in get_table, the actual error message uses an f-string {axis=} which will produce "axis=3 does not exist." This test will pass, but it's testing for a specific formatting detail. If the error message format changes in the future to remove the = sign (e.g., "axis 3 does not exist"), this test will break. Consider using a more flexible regex pattern like match=r"axis.*3.*does not exist" or ensure the error message format is intentionally part of the API contract.
| with pytest.raises(KeyError, match="axis=3 does not exist."): | |
| with pytest.raises(KeyError, match=r"axis.*3.*does not exist"): |
There was a problem hiding this comment.
who cares, if the code is updated, so should the test
polfeliu
force-pushed
the
INGK-1194-add-table-element-attribute-to-xdf-3-1
branch
from
5aadf99 to
a665f9d
Compare
polfeliu
changed the base branch from
develop
to
INGK-1223-remove-virtual-drive
…ve-virtual-drive' into INGK-1194-add-table-element-attribute-to-xdf-3-1
polfeliu
force-pushed
the
INGK-1194-add-table-element-attribute-to-xdf-3-1
branch
from
66882f8 to
35c839f
Compare
…4-add-table-element-attribute-to-xdf-3-1 # Conflicts: # poetry.lock
polfeliu
requested review from
GullinBustin,
MartinALL91,
julieta-prieto and
lucia-teruel
polfeliu
deleted the
INGK-1194-add-table-element-attribute-to-xdf-3-1
branch
Description
This PR parses the dictionary tables specified here: ingeniamc/xdf-specification#5
Please include a summary of the change and which issue is fixed.
Type of change
Please add a description and delete options that are not relevant.
Tests
Documentation
Please update the documentation.
[Unreleased]section of the CHANGELOG.