-
Notifications
You must be signed in to change notification settings - Fork 8
Support IncludeCondition in ParameterRefEntry elements #216
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?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 93.87% 94.11% +0.23%
==========================================
Files 46 48 +2
Lines 3594 3924 +330
==========================================
+ Hits 3374 3693 +319
- Misses 220 231 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
pre-commit.ci autofix |
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.
Pull Request Overview
This PR implements support for IncludeCondition in ParameterRefEntry elements within XTCE sequence containers. This allows parameters to be conditionally included in packet parsing based on the values of previously parsed parameters.
- Adds a new
ParameterRefEntryclass to handle conditional parameter inclusion - Updates
SequenceContainerto supportParameterRefEntryobjects in addition to directParameterreferences - Implements comprehensive unit and integration tests covering various conditional parsing scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| space_packet_parser/xtce/containers.py | Adds ParameterRefEntry class for conditional parameter parsing; updates SequenceContainer to handle ParameterRefEntry objects and store parameter lookup for runtime resolution |
| tests/unit/test_xtce/test_include_condition.py | Comprehensive unit tests for ParameterRefEntry parsing, XML serialization/deserialization, and conditional logic with various operators |
| tests/integration/test_include_condition_integration.py | Integration tests validating end-to-end parsing with XTCE definitions containing conditional parameters |
| tests/test_data/test_include_condition.xml | Test XTCE document defining a packet structure with conditional parameters for integration testing |
| # Store parameter lookup for use during parsing | ||
| _parameter_lookup: Optional[dict[str, parameters.Parameter]] = field(default=None, repr=False, compare=False) |
Copilot
AI
Nov 6, 2025
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.
[nitpick] The comment 'Store parameter lookup for use during parsing' should be placed inside the docstring for the field or as part of the class docstring explaining the purpose of _parameter_lookup. Inline comments for dataclass fields are better placed in the class-level docstring or as a docstring for the field itself.
| # LIMITATION: MatchCriteria interface expects a single object, but ComparisonList | ||
| # requires AND logic across multiple comparisons. For now, we only support single | ||
| # comparisons in ComparisonList. Full AND logic would require a new MatchCriteria | ||
| # subclass or modifying include_condition to accept a list of MatchCriteria. | ||
| # This follows the same limitation pattern used in restriction_criteria where | ||
| # multiple MatchCriteria are stored in a list on the SequenceContainer itself. |
Copilot
AI
Nov 6, 2025
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.
[nitpick] The comment describes a limitation but doesn't clearly state what the expected behavior is according to the XTCE spec. The comment should explicitly state that according to the XTCE specification, ComparisonList should have AND semantics across all comparisons, and link to the relevant section if possible.
| warnings.warn( | ||
| f"ComparisonList with {len(comparisons_list)} comparisons in IncludeCondition " | ||
| f"for parameter '{parameter_ref}'. Only the first comparison will be evaluated. " | ||
| f"Full AND logic for ComparisonList in IncludeCondition requires architectural " |
Copilot
AI
Nov 6, 2025
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.
[nitpick] The warning message should indicate the potential consequences of this limitation more clearly. Consider adding that this may result in incorrect packet parsing if the XTCE definition relies on multiple comparisons in the ComparisonList.
| f"Full AND logic for ComparisonList in IncludeCondition requires architectural " | |
| f"This may result in incorrect packet parsing if the XTCE definition relies on multiple comparisons " | |
| f"being evaluated with AND logic. Full AND logic for ComparisonList in IncludeCondition requires architectural " |
| # Since we don't fully parse it, we can't fully serialize it either | ||
| warnings.warn( | ||
| "RepeatEntry serialization is not fully implemented. " | ||
| "The RepeatEntry element will not be included in the XML output.", |
Copilot
AI
Nov 6, 2025
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.
The warning is issued during serialization but doesn't indicate whether this could cause issues when the XML is read back. Consider adding information about whether round-tripping (parse XML -> serialize -> parse again) will preserve the RepeatEntry information or if it will be lost.
| "The RepeatEntry element will not be included in the XML output.", | |
| "The RepeatEntry element will not be included in the XML output. " | |
| "If you parse XML containing RepeatEntry and then serialize it again, the RepeatEntry information will be lost.", |
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
for more information, see https://pre-commit.ci
f015995 to
4cac7d7
Compare
Description
Implements XTCE 5.3.4 IncludeCondition support for conditional parameter parsing based on previously-parsed values.
Implementation
New
ParameterRefEntryclass wraps parameter references with optionalIncludeConditionandRepeatEntry(placeholder)MatchCriteriainfrastructureNotImplementedErrorfor RepeatEntry during parseUpdated
SequenceContainerParameterRefEntryobjects only when IncludeCondition/RepeatEntry present (backward compatible)_parameter_lookupfor runtime parameter resolutionentry_list: list[Union[ParameterRefEntry, Parameter, SequenceContainer]]Example
Limitations
ComparisonList with multiple comparisons evaluates only first element (warns user). Full AND logic requires architectural changes to support multiple MatchCriteria per condition.
Checklist
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
www.omg.org/usr/bin/python3 /home/REDACTED/.local/bin/pytest --color=yes --cov --cov-report=term-missing(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>Support
IncludeConditioninParameterRefEntryelements</issue_title><issue_description>## Summary
Space Packet Parser supports much of the XTCE XML schema but the
IncludeConditionelement withinParameterRefEntryis not yet handled.XTCE Reference: Section 5.3.4 (p. 235) of CCSDS 660.1-G-2
IncludeConditionelements allow conditional inclusion of aParameterRefEntryin theEntryListof aSequenceContainer. The condition can only reference previously parsed parameters in the packet. During parsing, the partially-parsedpacketobject is available to look up these values.XTCE Example
In this example,
CheckSumis only parsed if the previously-parsedCSFlagparameter equals 1.Implementation
Current Architecture Limitation
Currently in
containers.pylines 111-131, we parseSequenceContainer/EntryListelements into a simpleentry_listof Parameter and SequenceContainer objects. This doesn't support:IncludeCondition)RepeatEntry)Create New
ParameterRefEntryClassCreate a new class in
containers.pyto represent theParameterRefEntryXML element with its optional child elements.Attributes:
parameter_ref: str- Name reference to the Parameterrepeat_entry: Optional[RepeatEntry]- Repeat information (raise NotImplementedError for now)include_condition: Optional[MatchCriteria]- Condition for inclusion (use existing MatchCriteria types from comparisons.py)Inheritance:
common.XmlObjectandcommon.Parseablefrom_xml()to parse the XML structureto_xml()for serializationparse()to handle conditional parsing logicParse Logic:
include_conditionexists, evaluate it using the current packet staterepeat_entryexists, raiseNotImplementedErrorwith message: "RepeatEntry is not currently supported in parsing"Scope Constraint (XTCE 5.8.4):
The
IncludeConditioncan only reference parameters that have already been parsed in the current container (previous entries in the EntryList). References to "future" parameters in the same container must be rejected or produce a warning.Update SequenceContainer.from_xml()
Modify the loop at lines 111-131 to create
ParameterRefEntryobjects instead of directly referencing Parameter objects:Add Warning for Unrecognized EntryList Elements
At the end of the if-elif chain (after line 131 in current code), add:
Change Type of
entry_listThe
entry_listattribute inSequenceContainershould be updated to:This replaces the current
list[Union[parameters.Parameter, SequenceContainer]].Backward Compatibility: The
SequenceContainer.parse()method must be updated to handleParameterRefEntryobjects, which will need to resolve their parameter references and evaluate conditions during parsing.Test Cases
IncludeConditioninParameterRefEntryelements #28💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.