Refactor state extraction to use typed StateRecord and enums #58
Conversation
- Introduced `StateRecord`, a typed NamedTuple, to replace dict-based state records. - Replaced raw integer state values with proper enum names using `ComponentStateCode` and `ComponentErrorCode`. - Improved internal structure by splitting `_extract_state_records()` and `_filter_alerts()` into cleaner, testable units. - Added `_resolve_enum_name()` to map proto values to readable enum names. - Updated unit tests accordingly for enum-based comparisons and removed deprecated test cases. This improves downstream use of state records, such as in alert visualisation and reporting. Signed-off-by: cyiallou - Costas <[email protected]>
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR refactors state extraction to use a strongly typed StateRecord named tuple and replaces raw integer state and error codes with enum-based names, improving clarity and testability.
- Introduced
StateRecord,_extract_state_records,_process_sample_group, and_resolve_enum_nameto replace dict-based logic and centralize enum resolution. - Updated
fetch_and_extract_state_durationsto use the new extraction and filtering functions. - Revised tests to use enum values and removed deprecated
delete_metests. - Updated release notes to document the switch to
StateRecordwith enum names.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_frequenz_reporting.py | Switched tests from raw ints to enum values; updated test logic |
| src/frequenz/reporting/_reporting.py | Added StateRecord, split extraction/filtering, and enum mapping |
| RELEASE_NOTES.md | Documented migration to StateRecord with descriptive enums |
Comments suppressed due to low confidence (1)
tests/test_frequenz_reporting.py:167
- The debug
printstatement should be removed from the test to avoid cluttering test output.
print(test_case["alert_states"])
| end_time=end_time, | ||
| resampling_period=resampling_period, | ||
| include_states=True, | ||
| include_bounds=False, |
There was a problem hiding this comment.
Pass include_states=True to _fetch_component_data so that state metrics are actually fetched for processing.
There was a problem hiding this comment.
LGTM at first glace. The naming likely to change with updates in client-common though (also a topic for assets api that I would like to discuss next week). If you need this now it is probably good to move along, but we will have to rename once changes from the 'api-common' side are pulled through.
This PR makes it easier to work with downstream because as it is now it accepts a list of integers as This code is used in the Alerts notebook on |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
The codebase requires `frequenz.client.common.enum_proto`, which was added in v0.3.3. Earlier versions lack this module, causing test failures in `pytest_min`. Bumping the minimum version ensures compatibility with `frequenz-repo-config`’s min-dependency testing and avoids runtime import errors. Signed-off-by: cyiallou - Costas <[email protected]>
github-actions
bot
added
the
part:tooling
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Closing as this code has been moved to I will open another PR that will remove the parts of the code that have been moved. |
2 participants
StateRecord, a typed NamedTuple, to replace dict-based state records.ComponentStateCodeandComponentErrorCode._extract_state_records()and_filter_alerts()into cleaner, testable units._resolve_enum_name()to map proto values to readable enum names.This improves downstream use of state records, such as in alert visualisation and reporting.