feat(domain): model Annex VII expected-lifetime parameters - #61
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully models Annex VII Part B expected-lifetime parameters for stationary storage and LMT batteries, including the implementation of schema v2.4.0 and appropriate privacy-level classifications. The architectural decision to use boxed types for large optional measurement blocks optimizes memory usage as intended.
While the code is up to standards according to Codacy, there is an implementation gap in the sector-battery plugin where the new numeric fields (throughput and cycles) are not yet covered by manual validation, potentially allowing negative values to bypass early checks. Additionally, some boilerplate in the test suite should be refactored to reduce maintenance overhead.
Test suggestions
- Verify that EV batteries are excluded from Annex VII Part B logic while stationary/LMT are included.
- Verify that unconditional items (throughput, cycles) are required in the schema while 'putting into service' is optional.
- Verify that individual harmful event types are optional within the HarmfulEvents block.
- Verify the privacy classification split between measured individual lifetime data and model-level design cycles.
- Confirm serialization and deserialization (round-trip) of the new ExpectedLifetime structures.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| // use data to persons with a legitimate interest. Two different fields, | ||
| // two different classes — collapsing them would either hide a public figure | ||
| // or publish an individual battery's usage history. | ||
| use crate::catalog::SectorCatalog; |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: This boilerplate setup is repeated across multiple battery classification tests. Consider refactoring it into a private helper function.
Try running the following prompt in your IDE agent:
Refactor the catalog and battery descriptor loading logic in
crates/dpp-domain/src/domain/sector/tests.rsinto a shared helper function. Use this helper instate_of_health_is_withheld_from_authoritiesandmeasured_lifetime_is_individual_but_the_design_figure_stays_publicto reduce duplication.
| assert!(battery.schema_versions.contains(&"2.2.0".to_string())); | ||
| assert!(battery.schema_versions.contains(&"2.3.0".to_string())); | ||
| assert!(battery.schema_versions.contains(&"2.4.0".to_string())); | ||
| // Current version is v2.2.0, which adds the Annex VII Part A state-of-health |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: This comment is outdated. Version 2.4.0 is now the current version and it introduces Annex VII Part B parameters. Consider updating the comment to reflect the current state of the catalog.
Annex VII Part B, completing the Annex VII pair started in #56.
Part B is narrower than Part A, and the difference is easy to miss
Part A names "electric vehicle batteries, stationary battery energy storage systems and LMT batteries". Part B names only "stationary battery energy storage systems and LMT batteries".
An electric-vehicle battery reports a state of health under Part A but no expected-lifetime parameter set at all.
annex_vii_part_b_applies_toencodes that, with a test asserting the asymmetry directly — it would otherwise be a silent over-collection.The parameters
ExpectedLifetimecarries all five Part B items: date of putting into service, energy throughput, capacity throughput, tracking of harmful events, and the number of full equivalent charge-discharge cycles.Optionality again follows the annex's own wording. Only item 1's "where appropriate" date of putting into service is qualified; items 2, 3 and 5 are unconditional and therefore required. Item 4 lists its harmful events "such as", so that set is illustrative, not closed — every member is optional, and an implementation tracking a further event type is conforming rather than extending.
HarmfulEventsis modelled accordingly.The disclosure split this exposes
Two fields that sound like one thing sit in different Annex XIII points:
expectedLifetimeCycles— the model-level design figureexpectedLifetime— measured, per physical batteryCollapsing them would either hide a figure the regulation makes public, or publish one battery's usage history. A test pins both classifications.
A size regression, fixed properly
Adding the block pushed
BatteryDatato 720 bytes — 224 more than the nextSectorDatavariant, past clippy'slarge_enum_variantthreshold. Boxing one field to duck under the limit would have been the wrong fix; both Annex VII measurement blocks are large, optional and populated only for batteries that actually report them, sostate_of_healthandexpected_lifetimeare both boxed. That is right regardless of the lint, and leaves headroom rather than sitting just under a threshold.Also in this PR
A one-line accuracy correction to #60, from its Codacy review: I wrote that
PassportRepositoryis the "whole" surface Art. 78(d) applies to, but resolver scan telemetry is processed data too. Now "primary", in both the port doc andCONFORMITY.md.Verification
just checkgreen: 827 tests, plugin suites included.just build-pluginsgreen. Schema v2.4.0; v1.0.0–v2.3.0 stay registered.