Skip to content

Add Pydantic-based config validation and type correction#310

Open
MaStr wants to merge 19 commits intomainfrom
claude/pydantic-config-conversion-Qc7UW
Open

Add Pydantic-based config validation and type correction#310
MaStr wants to merge 19 commits intomainfrom
claude/pydantic-config-conversion-Qc7UW

Conversation

@MaStr
Copy link
Copy Markdown
Owner

@MaStr MaStr commented Mar 17, 2026

Summary

Introduces comprehensive configuration validation using Pydantic models to eliminate scattered type conversion code throughout the codebase and fix Home Assistant addon issues where numeric values arrive as strings.

Key Changes

  • New Pydantic models (src/batcontrol/config_model.py):

    • BatcontrolConfig: Top-level configuration with validation for time_resolution_minutes (15 or 60) and loglevel
    • BatteryControlConfig, InverterConfig, UtilityConfig, MqttConfig, EvccConfig: Specialized config models for each subsystem
    • PvInstallationConfig, ConsumptionForecastConfig: Installation and forecast-specific models
    • validate_config() function: Entry point for validating raw config dicts and returning validated dicts
  • Type coercion at config load time:

    • String-to-int coercion for numeric fields (e.g., MQTT port, time resolution)
    • String-to-float coercion for decimal fields (e.g., VAT, fees, price differences)
    • Semicolon-separated string parsing for list fields (e.g., history_days="-7;-14;-21"[-7, -14, -21])
    • Legacy field renaming support (max_charge_ratemax_grid_charge_rate)
  • Integration with config loading (src/batcontrol/setup.py):

    • load_config() now calls validate_config() to validate and coerce types before returning the config dict
  • Simplified downstream code:

    • Removed manual type conversions in core.py (time resolution validation)
    • Removed manual string-to-float conversions in dynamictariff.py
    • Removed manual list parsing in consumption.py
    • Removed legacy field handling in inverter.py
  • Comprehensive test coverage (tests/batcontrol/test_config_model.py):

    • 40+ unit tests covering all config models and edge cases
    • Tests for HA addon string coercion scenarios
    • Tests for backward compatibility (legacy field names)
    • Tests for validation errors and constraints
  • Integration tests (tests/batcontrol/test_config_load_integration.py):

    • Tests for load_config() with Pydantic validation
    • Tests for actual YAML file loading and validation

Notable Implementation Details

  • Uses Pydantic's extra='allow' to preserve unknown fields for forward compatibility
  • Implements custom validators for complex parsing (semicolon-separated lists, legacy field renaming)
  • All numeric fields support string input, automatically coerced to proper types
  • Validation happens once at config load time, eliminating runtime type checks throughout the codebase
  • Maintains backward compatibility with existing config files and legacy field names

https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP

Closes #144

Copilot AI review requested due to automatic review settings March 17, 2026 18:50
@MaStr MaStr changed the title Add Pydantic-based config validation and type coercion Add Pydantic-based config validation and type correction Mar 17, 2026
@MaStr MaStr added enhancement New feature or request CodeQuality PRs fixing CodeQuality issues labels Mar 17, 2026
@MaStr MaStr added this to the 0.8.0 milestone Mar 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 introduces centralized configuration validation/coercion via new Pydantic models, aiming to handle Home Assistant “numbers as strings” issues at config load time and remove scattered runtime conversions across Batcontrol.

Changes:

  • Added src/batcontrol/config_model.py with Pydantic models and a validate_config() entrypoint.
  • Integrated Pydantic validation into load_config() and removed several downstream coercions/parsers (core time resolution, tariff float casting, HA consumption list parsing, inverter legacy rename).
  • Added unit/integration tests covering validation, coercion, backward compatibility, and load_config() behavior; added pydantic dependency.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/batcontrol/test_core.py Updates time-resolution tests to reflect validation happening before Batcontrol init.
tests/batcontrol/test_config_model.py Adds unit tests for Pydantic config models and coercion/validation behavior.
tests/batcontrol/test_config_load_integration.py Adds integration tests for YAML load_config() + validation.
src/batcontrol/setup.py Calls validate_config() during config loading.
src/batcontrol/inverter/inverter.py Removes legacy field/default handling; adds cache_ttl wiring for MQTT inverter.
src/batcontrol/forecastconsumption/consumption.py Removes semicolon-list parsing and int coercion (now expected from validation).
src/batcontrol/dynamictariff/dynamictariff.py Removes float() casting; assumes validated numeric types.
src/batcontrol/core.py Removes time-resolution string coercion/validation; assumes validated config.
src/batcontrol/config_model.py New Pydantic models + validate_config() for centralized validation/coercion.
pyproject.toml Adds pydantic as a dependency.

Copilot AI review requested due to automatic review settings March 17, 2026 19:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a centralized, Pydantic v2–based configuration validation layer so config type coercion/validation happens once at load time (helping address HA addon “numbers as strings” issues) and downstream modules can rely on properly typed config values.

Changes:

  • Introduces src/batcontrol/config_model.py with Pydantic models plus validate_config() and wires it into load_config().
  • Removes scattered runtime type conversions in core.py, dynamictariff.py, forecastconsumption/consumption.py, and legacy mapping in inverter.py.
  • Adds extensive unit + integration tests for config validation/coercion and updates existing tests accordingly.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/batcontrol/config_model.py New Pydantic models + validate_config() entry point to coerce/validate config.
src/batcontrol/setup.py Calls validate_config() during load_config() to centralize validation.
src/batcontrol/core.py Removes time_resolution_minutes coercion/validation, relying on validated config.
src/batcontrol/dynamictariff/dynamictariff.py Removes float() casting for utility numeric fields, relying on validated config.
src/batcontrol/forecastconsumption/consumption.py Removes manual parsing for history_days/history_weights (now intended to be handled by validation).
src/batcontrol/inverter/inverter.py Removes legacy rename/default handling (moved to Pydantic) and passes cache_ttl for MQTT inverter.
tests/batcontrol/test_config_model.py New unit tests for models, coercions, constraints, and validate_config().
tests/batcontrol/test_config_load_integration.py New integration tests verifying load_config() + YAML loading + validation behavior.
tests/batcontrol/test_core.py Updates time-resolution tests to reflect validation occurring before Batcontrol.__init__().
pyproject.toml Adds pydantic>=2.0,<3.0 dependency.
docs/pydantic-config-migration.md Migration notes, design decisions, and follow-ups.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds centralized configuration validation/coercion using Pydantic so config type fixes happen once at load time (addressing Home Assistant addon string-typed numeric values) and downstream modules can rely on normalized types.

Changes:

  • Introduces BatcontrolConfig and subsystem config models + validate_config() for type coercion and validation.
  • Integrates validation into load_config() and removes scattered manual coercions in core/tariff/consumption/inverter code paths.
  • Adds unit + integration tests and migration documentation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/batcontrol/config_model.py New Pydantic models + validate_config() to validate/coerce loaded config.
src/batcontrol/setup.py Calls validate_config() during config load.
src/batcontrol/core.py Removes local time resolution coercion/validation, assumes validated config.
src/batcontrol/dynamictariff/dynamictariff.py Removes float() casts, relies on validated float types.
src/batcontrol/forecastconsumption/consumption.py Simplifies list parsing; keeps fallback parsing for nested HA config.
src/batcontrol/inverter/inverter.py Removes legacy rename/default handling (now in Pydantic); passes cache_ttl.
tests/batcontrol/test_config_model.py New unit tests for models and coercion behavior.
tests/batcontrol/test_config_load_integration.py New integration tests for load_config() + YAML loading.
tests/batcontrol/test_core.py Updates time-resolution test to reflect validated-config expectations.
pyproject.toml Adds pydantic>=2.0,<3.0 runtime dependency.
docs/pydantic-config-migration.md Documents migration status, design decisions, and remaining follow-ups.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds centralized configuration validation/coercion using Pydantic to address Home Assistant add-on string-typed options and remove scattered runtime type conversions across Batcontrol.

Changes:

  • Introduces src/batcontrol/config_model.py Pydantic v2 models + validate_config() to validate/coerce config at load time.
  • Integrates validation into setup.load_config() and simplifies downstream modules (core.py, dynamictariff.py, inverter.py, forecastconsumption/consumption.py) by removing redundant casts/renames.
  • Adds extensive unit + integration tests for config validation and YAML load behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/batcontrol/config_model.py New Pydantic models + validate_config() that returns a coerced dict (model_dump(exclude_none=True)).
src/batcontrol/setup.py Calls validate_config() during config load before further checks.
src/batcontrol/core.py Removes local time_resolution_minutes coercion/validation and relies on validated config.
src/batcontrol/dynamictariff/dynamictariff.py Removes float casts for utility pricing parameters assuming validated types.
src/batcontrol/inverter/inverter.py Removes legacy rename/default handling (moved to Pydantic) and adds cache_ttl passthrough for MQTT inverter config.
src/batcontrol/forecastconsumption/consumption.py Adjusts parsing/coercion for nested HA config cases and ensures numeric types for annual consumption.
tests/batcontrol/test_config_model.py New unit tests covering model defaults, coercions, validators, and validate_config() output shape.
tests/batcontrol/test_config_load_integration.py New integration tests validating YAML load + Pydantic coercion through load_config().
tests/batcontrol/test_core.py Updates time-resolution test expectations to reflect load-time validation behavior.
pyproject.toml Adds pydantic>=2.0,<3.0 dependency.
docs/pydantic-config-migration.md Adds migration/status documentation for the new validation layer.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds centralized, Pydantic v2–based configuration validation/coercion at config load time to address Home Assistant “numbers as strings” issues and reduce scattered runtime type conversions across Batcontrol.

Changes:

  • Introduces BatcontrolConfig and subsystem config models in src/batcontrol/config_model.py, plus validate_config() returning a plain validated dict (exclude_none=True).
  • Integrates validation into setup.load_config() and removes/reduces downstream manual coercions (e.g., core.py, dynamictariff.py, inverter.py, forecastconsumption/consumption.py).
  • Adds extensive unit + integration tests covering coercion, defaults, validation errors, and YAML load behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/batcontrol/test_core.py Updates time-resolution tests to reflect validation happening at load time.
tests/batcontrol/test_config_model.py Adds unit tests for new Pydantic config models and coercion/validation behavior.
tests/batcontrol/test_config_load_integration.py Adds integration tests for load_config() validating/coercing YAML configs.
src/batcontrol/setup.py Calls validate_config() after YAML load and before PV installation checks.
src/batcontrol/inverter/inverter.py Removes legacy/default handling now covered by Pydantic; passes cache_ttl for MQTT inverter.
src/batcontrol/forecastconsumption/consumption.py Adjusts parsing/coercion to handle nested HA config edge cases and numeric strings.
src/batcontrol/dynamictariff/dynamictariff.py Removes float casts assuming config has already been coerced by Pydantic.
src/batcontrol/core.py Removes manual time_resolution_minutes coercion/validation and uses validated value.
src/batcontrol/config_model.py New Pydantic models and validate_config() entry point.
pyproject.toml Adds pydantic>=2.0,<3.0 dependency.
docs/pydantic-config-migration.md Documents migration status, design decisions, and remaining follow-ups.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 centralizes configuration validation/coercion by introducing Pydantic v2 models and wiring them into config loading, reducing scattered runtime type conversions (notably for Home Assistant add-on stringly-typed options).

Changes:

  • Added BatcontrolConfig + subsystem config models and a validate_config() entry point to validate/coerce raw config dicts at load time.
  • Integrated validation into setup.load_config() and simplified downstream modules (core, inverter factory, dynamic tariff factory, consumption forecast factory) by removing redundant casts/legacy handling.
  • Added extensive unit + integration tests for config validation and YAML loading, plus a migration note doc.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/batcontrol/config_model.py New Pydantic models + validation entry point for centralized config coercion.
src/batcontrol/setup.py Calls validate_config() during config load; adds basic YAML shape check.
src/batcontrol/core.py Removes local time_resolution_minutes coercion/validation and relies on validated config.
src/batcontrol/inverter/inverter.py Drops legacy key mutation/defaulting in factory; passes through cache_ttl.
src/batcontrol/dynamictariff/dynamictariff.py Removes float() casts for already-coerced utility config values.
src/batcontrol/forecastconsumption/consumption.py Keeps parsing for nested HA config dicts; adjusts coercions for numeric/list fields.
tests/batcontrol/test_config_model.py New unit tests for model defaults, coercion, constraints, and backwards-compat rename.
tests/batcontrol/test_config_load_integration.py New integration tests covering YAML load + validation behavior.
tests/batcontrol/test_core.py Updates time resolution tests to reflect validation now happening in load_config().
pyproject.toml Adds pydantic>=2,<3 dependency.
docs/pydantic-config-migration.md Documents migration status, design decisions, and remaining follow-ups.

Comment on lines +222 to +231
battery_control: BatteryControlConfig = BatteryControlConfig()
battery_control_expert: Optional[BatteryControlExpertConfig] = None
inverter: InverterConfig = InverterConfig()
utility: UtilityConfig
mqtt: Optional[MqttConfig] = None
evcc: Optional[EvccConfig] = None
pvinstallations: List[PvInstallationConfig]
consumption_forecast: ConsumptionForecastConfig = (
ConsumptionForecastConfig()
)
Comment on lines +91 to +93
# Coerce numeric fields that may arrive as strings from nested HA config
cache_ttl_hours = float(ha_config.get('cache_ttl_hours', 48.0))
multiplier = float(ha_config.get('multiplier', 1.0))
Comment on lines +93 to 95
# in config_model.py (must be int, must be 15 or 60)
self.time_resolution = config.get('time_resolution_minutes', 60)

@MaStr
Copy link
Copy Markdown
Owner Author

MaStr commented Apr 2, 2026

Round 5 Review Feedback - All items addressed

Addressed the 3 remaining comments from the latest review round:

1. Mutable defaults in BatcontrolConfig (✅ Fixed)

Comment: BatcontrolConfig used instantiated BaseModel objects as defaults (e.g., BatteryControlConfig(), InverterConfig(), ConsumptionForecastConfig()), which can cause cross-instance state leakage.

Action: Switched to Field(default_factory=BatteryControlConfig) / Field(default_factory=InverterConfig) / Field(default_factory=ConsumptionForecastConfig) — the recommended Pydantic best practice. While Pydantic v2 does deep-copy these instances, using default_factory is cleaner and more explicit.

2. float(None) TypeError in consumption.py (✅ Fixed)

Comment: float(ha_config.get('cache_ttl_hours', 48.0)) raises TypeError when the key exists with a null/None YAML value (because get() returns None rather than the fallback).

Action: Now explicitly checks for None before casting: _v = ha_config.get('cache_ttl_hours'); float(_v if _v is not None else 48.0). Same fix applied to multiplier.

3. Defensive validation in Batcontrol.__init__ (✅ Fixed)

Comment: __init__ now assumes time_resolution_minutes is already a valid int (15/60) from Pydantic validation, but direct instantiation without load_config() could cause a silent ZeroDivisionError.

Action: Added a minimal guard after config.get(...) that raises a clear ValueError if the value isn't 15 or 60. Keeps the validated-at-load-time design while giving a clear error for any direct construction without validation.

All 64 existing tests pass after these changes.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces centralized, Pydantic v2–based configuration validation/coercion at config-load time to remove scattered runtime type conversions and improve robustness (notably for Home Assistant add-on stringified numeric options).

Changes:

  • Added BatcontrolConfig and subsystem config models plus a validate_config() entry point to validate/coerce raw config dicts.
  • Integrated validation into setup.load_config() and simplified downstream modules by removing redundant casts/parsing.
  • Added extensive unit + integration test coverage for validation and load-time behavior, plus migration documentation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/batcontrol/config_model.py New Pydantic models + validate_config() to validate/coerce config at load time
src/batcontrol/setup.py Calls validate_config() after YAML load and adds basic YAML-shape validation
src/batcontrol/core.py Removes string→int coercion logic for time_resolution_minutes, relies on validated config
src/batcontrol/dynamictariff/dynamictariff.py Removes redundant float() casting for utility price parameters and tariff zones
src/batcontrol/forecastconsumption/consumption.py Keeps targeted parsing/coercion for nested HA config edge-cases; coerces CSV annual consumption
src/batcontrol/inverter/inverter.py Removes legacy rename/default handling (now in Pydantic) and passes cache_ttl to MQTT inverter
tests/batcontrol/test_config_model.py New unit tests covering model defaults, coercion, constraints, legacy rename
tests/batcontrol/test_config_load_integration.py New integration tests for load_config() + dummy config file loading
tests/batcontrol/test_core.py Updates time-resolution tests to reflect validation happening before Batcontrol.__init__()
pyproject.toml Adds pydantic>=2.0,<3.0 dependency
docs/pydantic-config-migration.md Migration/status notes and remaining follow-ups

Comment on lines +72 to +77
if not isinstance(config, dict):
raise RuntimeError(f'Configfile {configfile} is empty or not a valid YAML mapping')

# Validate and coerce types via Pydantic before any other checks
config = validate_config(config)

Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_config() now calls validate_config(), which can raise pydantic.ValidationError, but the function docstring (and callers like main.py) still imply RuntimeError-only behavior. Consider either (a) catching ValidationError and re-raising RuntimeError with a user-friendly message, or (b) updating the docstring/type contract and ensuring the CLI handles ValidationError cleanly (no stack trace for config mistakes).

Copilot uses AI. Check for mistakes.
"""Write config dict to a YAML file and return the path."""
path = os.path.join(tmpdir, 'test_config.yaml')
with open(path, 'w', encoding='UTF-8') as f:
yaml.dump(config_dict, f)
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests write YAML using yaml.dump(), which can emit Python-specific tags for non-primitive objects and is less constrained than safe_dump(). Using yaml.safe_dump() here would better match load_config()'s yaml.safe_load() behavior and avoid surprising serialization output if test fixtures evolve.

Suggested change
yaml.dump(config_dict, f)
yaml.safe_dump(config_dict, f)

Copilot uses AI. Check for mistakes.
@MaStr
Copy link
Copy Markdown
Owner Author

MaStr commented Apr 2, 2026

Round 6 Review Feedback - All items addressed

1. load_config() docstring/callers expect RuntimeError but ValidationError could escape (✅ Fixed)

Comment: validate_config() can raise pydantic.ValidationError, but load_config()'s docstring and callers like __main__.py only expect RuntimeError.

Action: Wrapped validate_config() call in a try/except ValidationError that re-raises as RuntimeError with the validation message included. Updated the docstring to document that invalid/malformed config raises RuntimeError. Updated the integration test that was expecting ValidationError to now expect RuntimeError, and removed the now-unused ValidationError import.

2. yaml.dump() in tests should be yaml.safe_dump() (✅ Fixed)

Comment: Test helper used yaml.dump() which can emit Python-specific tags, inconsistent with load_config()'s use of yaml.safe_load().

Action: Changed to yaml.safe_dump() to match the production code's use of yaml.safe_load() — ensures tests write YAML that is identical in form to what real config files would contain.

All 64 tests pass.

@MaStr MaStr requested a review from Copilot April 2, 2026 14:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

@@ -66,9 +71,17 @@ def load_config(configfile:str) -> dict:

config = yaml.safe_load(config_str)
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_config() calls yaml.safe_load() directly; malformed YAML will raise a yaml.YAMLError, not the RuntimeError promised by the docstring. Consider catching yaml.YAMLError around the load and re-raising as RuntimeError with a clear message (and from exc).

Suggested change
config = yaml.safe_load(config_str)
try:
config = yaml.safe_load(config_str)
except yaml.YAMLError as exc:
raise RuntimeError(f'Configfile {configfile} is not valid YAML: {exc}') from exc

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +57
type: str = 'dummy'
address: Optional[str] = None
user: Optional[str] = None
password: Optional[str] = None
max_grid_charge_rate: float = 5000
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address/user/password are optional here, but Inverter.create_inverter() later subscripts them (e.g. config['address']) for fronius_gen24. Because validate_config() uses model_dump(exclude_none=True), YAML nulls like address: will be dropped and turn into KeyError at runtime. Add a type-dependent validator to require these fields when type='fronius_gen24' (and fail validation with a clear error).

Copilot uses AI. Check for mistakes.
Comment on lines +65 to +70
# MQTT inverter fields
capacity: Optional[int] = None
min_soc: Optional[int] = None
max_soc: Optional[int] = None
base_topic: Optional[str] = None
cache_ttl: int = 120
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MQTT inverter fields like capacity are optional in the model, but Inverter.create_inverter() uses config['capacity'] for type='mqtt', which will raise KeyError if omitted (and exclude_none=True will also drop nulls). Consider validating required MQTT fields when type='mqtt' so config errors are caught at load time instead of during runtime inverter creation.

Copilot uses AI. Check for mistakes.
Comment on lines +151 to +155
name: str = ''
type: Optional[str] = None
lat: Optional[float] = None
lon: Optional[float] = None
declination: Optional[float] = None
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PvInstallationConfig.name defaults to an empty string, which means a PV installation missing the name key will still pass validation and end up with name=''. Downstream code uses unit['name'] as a cache key; an empty/default name can silently collide across installations and hide config mistakes. Consider making name required (no default) and/or enforcing min_length=1.

Copilot uses AI. Check for mistakes.
@MaStr
Copy link
Copy Markdown
Owner Author

MaStr commented Apr 2, 2026

Round 7 Review Feedback - All 4 items addressed

1. yaml.YAMLError not caught in load_config() (✅ Fixed)

Comment: yaml.safe_load() can raise yaml.YAMLError for malformed YAML, but the docstring promises only RuntimeError.

Action: Added try/except yaml.YAMLError around the yaml.safe_load() call, re-raising as RuntimeError with a clear message. Now all failure modes in load_config() raise RuntimeError consistently.

2. InverterConfig missing type-dependent required fields for fronius_gen24 (✅ Fixed)

Comment: address, user, password are optional in the model but subscripted directly by create_inverter() for fronius_gen24, causing KeyError at runtime if omitted.

Action: Added model_validator(mode='after') to InverterConfig that enforces address, user, password are non-None when type='fronius_gen24'. Validation fails at config load time with a clear error message.

3. InverterConfig missing type-dependent required fields for mqtt (✅ Fixed)

Comment: capacity is optional in the model but subscripted directly by create_inverter() for type='mqtt'.

Action: Same validator enforces capacity is non-None when type='mqtt'.

4. PvInstallationConfig.name should be required (✅ Fixed)

Comment: name defaulted to '', which passes validation silently and can cause cache key collisions across PV installations.

Action: Removed the default — name is now required. All existing configs and tests already provide it.

Added 5 new tests for all new validation paths. 69 tests pass total.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

@MaStr
Copy link
Copy Markdown
Owner Author

MaStr commented Apr 2, 2026

Round 13 Review Feedback - Addressed

timezone should be required, not defaulting to 'Europe/Berlin' (✅ Fixed)

Comment: timezone: str = 'Europe/Berlin' silently applies the wrong timezone when a user forgets to set it, causing incorrect scheduling/price-window alignment. The dummy config explicitly documents it as "not optional".

Action: Removed the default — timezone: str is now required. A missing timezone now fails at config load time with a clear ValidationError. All existing tests already provide timezone, so no test updates were needed.

76 tests pass.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

claude and others added 18 commits April 2, 2026 19:15
Introduces Pydantic models for all configuration sections that validate
and coerce types at load time, fixing HA addon string-to-int/float
issues (e.g. MQTT/EVCC port, time_resolution_minutes, vat/fees/markup).

- Add config_model.py with typed models for all config sections
- Integrate validate_config() into load_config() in setup.py
- Remove manual type coercion from core.py and dynamictariff.py
- Add 33 new tests covering model validation and type coercion
- Add pydantic>=2.0 dependency

https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP
…el validation

- Add field_validators for history_days/history_weights semicolon string
  parsing (e.g. "-7;-14;-21" -> [-7, -14, -21]) from HA addon
- Add model_validator for legacy max_charge_rate -> max_grid_charge_rate rename
- Add loglevel validator (must be debug/info/warning/error, normalized to lowercase)
- Remove manual type coercion from consumption.py and inverter.py
- Add 10 new tests (367 total passing)

https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP
- Add cache_ttl field to InverterConfig Pydantic model (used by MQTT inverter)
- Fix pre-existing bug: cache_ttl was not passed through in inverter
  factory's MQTT path, always falling back to default 120

https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP
Documents what was done, what remains, config model reference,
and HA addon coercion table.

https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP
…default

Fixes all 5 review comments from Copilot:

1. Use model_dump(exclude_none=True) so None optional fields don't appear
   as keys in the output dict. Downstream code checks key presence
   (e.g. 'csv' in config, required_fields checks) and would break with
   None values present.

2. Make pvinstallations required (no default) - Pydantic now raises
   ValidationError if missing, instead of silently defaulting to [].

3. Give cache_ttl a real default (120) instead of Optional[None] to
   prevent None propagating into TTLCache.

4. Reorder load_config() to run validate_config() before the
   pvinstallations check, so Pydantic provides clear error messages.

5. tariff_zone_* None issue resolved by exclude_none=True.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes 2 valid review comments (3 others were already fixed):

1. Restore history_days/history_weights parsing in consumption factory
   for the nested homeassistant_api dict case. Pydantic only validates
   top-level ConsumptionForecastConfig fields; when values come from
   the nested homeassistant_api dict they bypass Pydantic validation
   and may still be semicolon-separated strings.

2. Copy input dict in InverterConfig model_validator before mutating
   via pop(). Prevents surprising side effects on callers who reuse
   the same dict.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cion, lint

Fixes 4 new review comments:

1. Make vat/fees/markup Optional[float]=None in UtilityConfig so
   downstream required_fields checks in dynamictariff.py can detect
   missing config (previously defaults of 0.0 masked misconfigurations).

2. Add float() coercion for cache_ttl_hours, multiplier, and
   annual_consumption in consumption factory for the nested
   homeassistant_api/csv dict paths that bypass Pydantic validation.

3. Remove unused import tempfile from test_config_load_integration.py.

4. Remove unused import BatteryControlExpertConfig from test_config_model.py.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cription

- vat/fees/markup are Optional[float]=None, not float=0.0
- Remove nonexistent 'entsoe' provider, list actual providers
- Clarify that semicolon parsing was kept for nested HA config case
- Note exclude_none=True in design decisions
- Remove nonexistent sungrow/huawei inverter types

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lass

- setup.py: raise RuntimeError when yaml.safe_load() returns None (empty
  or non-mapping YAML) before calling validate_config(), giving a clear
  error instead of a TypeError from **None unpacking
- test_core.py: rename TestTimeResolutionString -> TestTimeResolutionValidation
  and update docstring to reflect tests use int values (string coercion
  is covered in test_config_model.py)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sive validation

- Use Field(default_factory=...) for nested model defaults in BatcontrolConfig
  to follow Pydantic best practices (even though v2 deep-copies, this is cleaner)
- Guard against None values before float() cast in consumption.py HA config
  (YAML null values would previously raise TypeError)
- Add defensive time_resolution_minutes validation in Batcontrol.__init__
  so direct instantiation without load_config() gives a clear error

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n tests

- Catch pydantic.ValidationError in load_config() and re-raise as RuntimeError
  so callers (e.g. __main__.py) continue to see only RuntimeError as documented
- Update docstring to reflect that malformed config raises RuntimeError
- Replace yaml.dump() with yaml.safe_dump() in integration test helper to match
  load_config()'s use of yaml.safe_load() and avoid Python-specific YAML tags
- Update test expecting ValidationError to expect RuntimeError (new behavior)
- Remove now-unused ValidationError import from integration test file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion, name required

- Wrap yaml.safe_load() in try/except yaml.YAMLError → RuntimeError so malformed
  YAML gives a clean error consistent with the rest of load_config()
- Add type-dependent model_validator to InverterConfig: fronius_gen24 requires
  address/user/password; mqtt requires capacity — prevents KeyError at runtime
- Make PvInstallationConfig.name required (no default) to prevent empty-string
  cache key collisions across PV installations
- Add 5 new tests covering all new validation paths

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fix comment

- Change max_grid_charge_rate/max_pv_charge_rate/min_pv_charge_rate to int in
  InverterConfig — they represent watts and are used with %d formatting in logs
- Sanitize ValidationError in load_config(): use exc.errors() with only field path
  and message (no input_value) to avoid leaking secrets like inverter passwords
- Fix misleading comment in consumption.py: clarify it's an explicit None check
  (not 'or default') so it's clear why 0 is handled correctly as a valid value
- Update test asserting max_grid_charge_rate is float → int

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
max_grid_charge_rate/max_pv_charge_rate/min_pv_charge_rate are int in the
model (watts); update the field reference tree and coercion table accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After changing InverterConfig power rate fields to int in round 8,
three test assertions still compared against float literals (3000.0, 4000.0).
Update them to int and add isinstance(_, int) checks to catch regressions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nual_consumption None

- InverterConfig.validate_type_required_fields: reject empty/whitespace strings
  for fronius_gen24 address/user/password (not just None), and reject
  capacity <= 0 for mqtt (0 or negative is invalid for downstream calculations)
- consumption.py CSV factory: guard annual_consumption against None value
  (YAML null) with 'or 0' pattern — consistent with HA config guard added earlier
- Add 2 new tests: mqtt capacity=0 rejected; fronius empty-string fields rejected
- Update test match string to align with new error message wording

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…equired fields

- PvInstallationConfig: add field_validator for name to reject empty/whitespace
  strings (used as cache keys in forecastsolar — empty would cause collisions);
  strip leading/trailing whitespace from valid names
- UtilityConfig: add model_validator(mode='after') to enforce provider-specific
  required fields at config load time instead of at DynamicTariff runtime:
  * awattar_at/awattar_de: require vat, markup, fees
  * energyforecast/energyforecast_96: require vat, markup, fees, apikey
  * tibber: require apikey
  * evcc: require url
  * tariff_zones: require tariff_zone_1, zone_1_hours, tariff_zone_2, zone_2_hours
- Update all affected tests to provide required fields for their provider type
- Add 6 new tests covering name whitespace rejection, name stripping, and
  provider-specific validation errors (awattar, tibber, evcc)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The dummy config documents timezone as "not optional" and core logic
uses it for scheduling/price-window alignment. Silently defaulting to
'Europe/Berlin' masks misconfigured setups. Now fails fast at config
load with a clear validation error when timezone is absent.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@MaStr MaStr force-pushed the claude/pydantic-config-conversion-Qc7UW branch from d1b7f58 to eb9c525 Compare April 2, 2026 17:15
- UtilityConfig: update comment to reflect that vat/fees/markup are now
  validated by validate_provider_required_fields, not by downstream code
- dynamictariff.py: add note that required_fields checks are secondary
  safety guards — primary validation is now in Pydantic at load time

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 2, 2026 17:18
@MaStr
Copy link
Copy Markdown
Owner Author

MaStr commented Apr 2, 2026

Post-rebase local review

Rebased on latest main (2 new commits: evcc Solar-Prognose fix #312, codecov bump #313). All 397 tests pass cleanly.

Local review found two stale comments addressed in a housekeeping commit:

  • UtilityConfig comment was still saying vat/fees/markup rely on "downstream checks in dynamictariff.py" — updated to reflect that validate_provider_required_fields is now the primary guard at config load time
  • dynamictariff.py runtime required_fields checks had no comment explaining they're now redundant for known providers — added a note that they're secondary safety guards, with Pydantic as the primary validation

No functional changes. 397 tests pass.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CodeQuality PRs fixing CodeQuality issues enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move config DataModel to Pydantic

3 participants