Dev baseline #5
Merged
Dev baseline #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
update ruff.toml
modified: .gitignore modified: custom_components/utility_meter_evolved/__init__.py modified: custom_components/utility_meter_evolved/config_flow.py modified: custom_components/utility_meter_evolved/const.py modified: custom_components/utility_meter_evolved/select.py modified: custom_components/utility_meter_evolved/sensor.py new file: custom_components/utility_meter_evolved/strings copy.json modified: custom_components/utility_meter_evolved/strings.json modified: custom_components/utility_meter_evolved/translations/en.json
Reviewer's GuideRefactors config and options flows to leverage Home Assistant’s ConfigFlow API for both cron-based and predefined utility meter cycles with dynamic schema builders; improves validation (catching CronSimError and merging state restore errors); extends sensor state persistence for cron patterns and source IDs; filters out the “total” tariff in select entities with a warning; and adds a cron test script for pattern validation. Sequence diagram for the new ConfigFlow and OptionsFlow interactionsequenceDiagram
actor User
participant ConfigFlow as UtilityMeterEvolvedCustomConfigFlow
participant OptionsFlow as OptionsFlowHandler
participant HomeAssistant as Home Assistant
User->>ConfigFlow: Start configuration (async_step_user)
ConfigFlow->>HomeAssistant: Validate source sensor state
HomeAssistant-->>ConfigFlow: State info
ConfigFlow->>ConfigFlow: Validate input, choose flow
alt Cron config
ConfigFlow->>ConfigFlow: async_step_cron
ConfigFlow->>HomeAssistant: Validate cron pattern
HomeAssistant-->>ConfigFlow: Validation result
ConfigFlow->>HomeAssistant: async_create_entry (options)
else Predefined config
ConfigFlow->>ConfigFlow: async_step_predefined
ConfigFlow->>HomeAssistant: Validate predefined config
HomeAssistant-->>ConfigFlow: Validation result
ConfigFlow->>HomeAssistant: async_create_entry (options)
end
User->>OptionsFlow: Modify options (async_step_init)
OptionsFlow->>HomeAssistant: Validate calc sensor state
HomeAssistant-->>OptionsFlow: State info
OptionsFlow->>HomeAssistant: async_create_entry (updated options)
Class diagram for updated ConfigFlow and OptionsFlow structureclassDiagram
class UtilityMeterEvolvedCustomConfigFlow {
+data: Optional[Dict[str, Any]]
+async_step_user(user_input)
+async_step_cron(user_input)
+async_step_predefined(user_input)
+_validate_state(state)
+async_get_options_flow(config_entry)
}
class OptionsFlowHandler {
+config_entry: ConfigEntry
+options_schema
+async_step_init(user_input)
+_validate_state(state)
}
UtilityMeterEvolvedCustomConfigFlow --|> ConfigFlow
OptionsFlowHandler --|> OptionsFlow
OptionsFlowHandler o-- ConfigEntry
UtilityMeterEvolvedCustomConfigFlow ..> OptionsFlowHandler : creates
Class diagram for UtilityMeterSensor and state persistence changesclassDiagram
class UtilityMeterSensor {
-_unrecorded_attributes: frozenset
+extra_state_attributes()
+_change_status(tariff)
}
UtilityMeterSensor : +_unrecorded_attributes = {ATTR_NEXT_RESET, CONF_CRON_PATTERN, CONF_METER_TYPE, ATTR_SOURCE_ID}
UtilityMeterSensor : +extra_state_attributes() adds cron_pattern, meter_type, source_id
Class diagram for Select entity tariff filteringclassDiagram
class UtilityMeterTariffSelect {
+options: list[str]
+current_option: str | None
}
UtilityMeterTariffSelect : +options filters out 'total' with warning
Flow diagram for dynamic option schema creationflowchart TD
A[User selects config type] -->|cron| B[create_option_schema_cron]
A -->|predefined| C[create_option_schema_predefined]
B --> D[Options schema for cron cycles]
C --> E[Options schema for predefined cycles]
D --> F[OptionsFlowHandler uses schema]
E --> F
F --> G[Show options form to user]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @cabberley - I've reviewed your changes - here's some feedback:
- Remove the
temp/crontest.pyscratch file from the commit, as it doesn’t belong in the production codebase. - The
create_option_schema_cronandcreate_option_schema_predefinedfunctions duplicate a lot of schema logic and manually merge.schemadictionaries—consider extracting common fields and building schemas more declaratively to DRY up the code. - There’s significant duplication between the user, cron, and predefined flow steps—extract the shared validation logic into helper methods to simplify the config and options flow implementations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the `temp/crontest.py` scratch file from the commit, as it doesn’t belong in the production codebase.
- The `create_option_schema_cron` and `create_option_schema_predefined` functions duplicate a lot of schema logic and manually merge `.schema` dictionaries—consider extracting common fields and building schemas more declaratively to DRY up the code.
- There’s significant duplication between the user, cron, and predefined flow steps—extract the shared validation logic into helper methods to simplify the config and options flow implementations.
## Individual Comments
### Comment 1
<location> `custom_components/utility_meter_evolved/config_flow.py:140` </location>
<code_context>
+def create_option_schema_cron(data):
+ """Create the options schema for cron cycles."""
+
+ if data[CONF_SOURCE_CALC_SENSOR] is None:
+ cron_option_schema_1 = vol.Schema(
+ {
+ vol.Required(CONF_SOURCE_SENSOR,
+ default=data[CONF_SOURCE_SENSOR]): selector.EntitySelector(
+ selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
+ ),
+ vol.Optional(CONF_SOURCE_CALC_SENSOR ): selector.EntitySelector(
+ selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
+ ),
+ }
+ )
+ else:
+ cron_option_schema_1 = vol.Schema(
+ {
</code_context>
<issue_to_address>
Repeated schema construction logic could be refactored.
Extract the shared schema-building logic from both functions into a helper to avoid duplication and simplify future maintenance.
Suggested implementation:
```python
def _build_sensor_entity_schema(data):
"""Helper to build the shared entity selector schema."""
schema = {
vol.Required(CONF_SOURCE_SENSOR, default=data.get(CONF_SOURCE_SENSOR)): selector.EntitySelector(
selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
),
vol.Optional(CONF_SOURCE_CALC_SENSOR): selector.EntitySelector(
selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
),
}
return schema
def create_option_schema_cron(data):
"""Create the options schema for cron cycles."""
```
```python
schema = _build_sensor_entity_schema(data)
schema[vol.Required(CONF_METER_PERIODICALLY_RESETTING)] = selector.BooleanSelector()
return vol.Schema(schema)
```
</issue_to_address>
### Comment 2
<location> `custom_components/utility_meter_evolved/config_flow.py:268` </location>
<code_context>
+ vol.Optional(CONF_SOURCE_CALC_SENSOR): selector.EntitySelector(
+ selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
+ ),
+ vol.Required(CONF_CONFIG_TYPE, default=[CONF_CONFIG_PREDEFINED]): selector.SelectSelector(
+ selector.SelectSelectorConfig(
+ options=CONFIG_TYPES,
+ translation_key=CONF_CONFIG_TYPE,
+ mode=selector.SelectSelectorMode.LIST,
+ custom_value =False,
+ multiple=False
+ ),
+ ),
</code_context>
<issue_to_address>
Default value for CONF_CONFIG_TYPE is a list, but mode is not multiple.
Since multiple=False, the default should be a string (e.g., CONF_CONFIG_PREDEFINED) rather than a list to avoid validation issues.
</issue_to_address>
### Comment 3
<location> `custom_components/utility_meter_evolved/sensor.py:143` </location>
<code_context>
source_entity_id,
)
- cron_pattern = None
+ cron_pattern = config_entry.options[CONF_CRON_PATTERN]
delta_values = config_entry.options[CONF_METER_DELTA_VALUES]
meter_offset = timedelta(days=config_entry.options[CONF_METER_OFFSET])
</code_context>
<issue_to_address>
Direct access to config_entry.options[CONF_CRON_PATTERN] may raise KeyError.
This key may be missing for predefined cycles, causing a KeyError. Use .get(CONF_CRON_PATTERN) with a default or add a guard to handle missing keys.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+140
to
+149
| if data[CONF_SOURCE_CALC_SENSOR] is None: | ||
| cron_option_schema_1 = vol.Schema( | ||
| { | ||
| vol.Required(CONF_SOURCE_SENSOR, | ||
| default=data[CONF_SOURCE_SENSOR]): selector.EntitySelector( | ||
| selector.EntitySelectorConfig(domain=SENSOR_DOMAIN), | ||
| ), | ||
| vol.Optional(CONF_SOURCE_CALC_SENSOR ): selector.EntitySelector( | ||
| selector.EntitySelectorConfig(domain=SENSOR_DOMAIN), | ||
| ), |
There was a problem hiding this comment.
suggestion: Repeated schema construction logic could be refactored.
Extract the shared schema-building logic from both functions into a helper to avoid duplication and simplify future maintenance.
Suggested implementation:
def _build_sensor_entity_schema(data):
"""Helper to build the shared entity selector schema."""
schema = {
vol.Required(CONF_SOURCE_SENSOR, default=data.get(CONF_SOURCE_SENSOR)): selector.EntitySelector(
selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
),
vol.Optional(CONF_SOURCE_CALC_SENSOR): selector.EntitySelector(
selector.EntitySelectorConfig(domain=SENSOR_DOMAIN),
),
}
return schema
def create_option_schema_cron(data):
"""Create the options schema for cron cycles.""" schema = _build_sensor_entity_schema(data)
schema[vol.Required(CONF_METER_PERIODICALLY_RESETTING)] = selector.BooleanSelector()
return vol.Schema(schema)
Comment on lines
+268
to
+274
| vol.Required(CONF_CONFIG_TYPE, default=[CONF_CONFIG_PREDEFINED]): selector.SelectSelector( | ||
| selector.SelectSelectorConfig( | ||
| options=CONFIG_TYPES, | ||
| translation_key=CONF_CONFIG_TYPE, | ||
| mode=selector.SelectSelectorMode.LIST, | ||
| custom_value =False, | ||
| multiple=False |
There was a problem hiding this comment.
issue (bug_risk): Default value for CONF_CONFIG_TYPE is a list, but mode is not multiple.
Since multiple=False, the default should be a string (e.g., CONF_CONFIG_PREDEFINED) rather than a list to avoid validation issues.
| @@ -140,7 +140,7 @@ async def async_setup_entry( | |||
| source_entity_id, | |||
| ) | |||
|
|
|||
| cron_pattern = None | |||
There was a problem hiding this comment.
issue (bug_risk): Direct access to config_entry.options[CONF_CRON_PATTERN] may raise KeyError.
This key may be missing for predefined cycles, causing a KeyError. Use .get(CONF_CRON_PATTERN) with a default or add a guard to handle missing keys.
Comment on lines
+492
to
+495
| if CONF_REMOVE_CALC_SENSOR in user_input: | ||
| if user_input[CONF_REMOVE_CALC_SENSOR]: | ||
| # Remove the calc sensor from the options. | ||
| user_input[CONF_SOURCE_CALC_SENSOR] = None |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if CONF_REMOVE_CALC_SENSOR in user_input: | |
| if user_input[CONF_REMOVE_CALC_SENSOR]: | |
| # Remove the calc sensor from the options. | |
| user_input[CONF_SOURCE_CALC_SENSOR] = None | |
| if CONF_REMOVE_CALC_SENSOR in user_input and user_input[CONF_REMOVE_CALC_SENSOR]: | |
| user_input[CONF_SOURCE_CALC_SENSOR] = None | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add flexible cycle configuration with cron and predefined modes, refactor config and options flows, enhance validation, and expose additional attributes in sensors.
New Features:
Bug Fixes:
Enhancements: