Conversation
- Remove all unused type: ignore comments - Eliminate redundant type casts and assertions - Improve docstrings and comments for clarity - Document architectural constraints (field ordering for validators) - Add comprehensive logging for field ordering validation - Fix all ruff linting violations - Fix all mypy type checking errors - All 387 tests pass with 100% coverage maintained - All CI checks pass (lint, format, type checking) No functional changes - purely code quality and maintainability improvements.
There was a problem hiding this comment.
Pull request overview
This PR refactors the unit conversion system to support dynamic metric/imperial unit selection based on the device's temperature_type setting. Previously, all temperatures were converted to Fahrenheit. Now the system respects the device's configured unit preference (Celsius or Fahrenheit) and applies appropriate conversions for temperatures, flow rates, and volumes.
Changes:
- Implemented dynamic unit conversion using Pydantic
WrapValidatorthat checkstemperature_typefield - Added
from_celsiusandfrom_preferredfactory methods to temperature classes - Fixed type annotations throughout the codebase to resolve mypy errors
- Updated test fixtures and added comprehensive unit switching tests
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_unit_switching.py |
New test file validating dynamic Celsius/Fahrenheit conversions and flow rate/volume unit handling |
tests/test_temperature_converters.py |
Added tests for new from_celsius and from_preferred factory methods |
tests/conftest.py |
Replaced placeholder content with comprehensive test fixture for DeviceStatus |
src/nwp500/temperature.py |
Added to_preferred, from_celsius, and from_preferred methods to Temperature classes |
src/nwp500/models.py |
Moved temperature_type field to top of models and switched to WrapValidator for dynamic conversions |
src/nwp500/converters.py |
Implemented wrap validators for temperature, flow rate, and volume conversions |
src/nwp500/cli/output_formatters.py |
Updated unit suffix logic to display correct units based on device preference |
src/nwp500/mqtt/*.py |
Added type annotations to resolve mypy errors |
src/nwp500/field_factory.py |
Removed unnecessary casts and added explicit type annotations |
src/nwp500/encoding.py |
Added variable declarations to fix unbound variable errors |
src/nwp500/auth.py |
Added type annotations and explicit casts for dict operations |
src/nwp500/cli/handlers.py |
Fixed type annotations for JSON parsing |
| "str_enum_validator", | ||
| "half_celsius_to_preferred", | ||
| "deci_celsius_to_preferred", | ||
| "flow_rate_to_preferred", |
There was a problem hiding this comment.
The __all__ export list is missing volume_to_preferred which is defined in the module and used in models.py. This function should be included in the public API exports.
| "flow_rate_to_preferred", | |
| "flow_rate_to_preferred", | |
| "volume_to_preferred", |
src/nwp500/cli/output_formatters.py
Outdated
|
|
||
| def _add_numeric_item( | ||
| items: list[tuple[str, str, str]], | ||
| items: list[tuple[str, str, Any]], |
There was a problem hiding this comment.
The type annotation changed from list[tuple[str, str, str]] to list[tuple[str, str, Any]] which weakens type safety. The third element should be typed as str since it represents formatted values that are always strings in the function's usage.
| items: list[tuple[str, str, Any]], | |
| items: list[tuple[str, str, str]], |
src/nwp500/models.py
Outdated
| class DeviceStatus(NavienBaseModel): | ||
| """Represents the status of the Navien water heater device.""" | ||
|
|
||
| # CRITICAL: temperature_type must be first. Wrap validators need it in |
There was a problem hiding this comment.
The comment states temperature_type must be "first" but it's actually just required to be defined before temperature fields that depend on it. Consider clarifying to "must be defined before any temperature fields" to be more accurate about the ordering constraint.
| # CRITICAL: temperature_type must be first. Wrap validators need it in | |
| # CRITICAL: temperature_type must be defined before any temperature | |
| # fields that depend on it. Wrap validators need it in |
| def half_celsius_to_preferred( | ||
| value: Any, handler: ValidatorFunctionWrapHandler, info: ValidationInfo | ||
| ) -> float: |
There was a problem hiding this comment.
The docstring states "handler: Pydantic next validator handler. Not used here" but this is misleading. WrapValidators should either use the handler or be BeforeValidators instead. If the handler is intentionally unused, explain why a WrapValidator is required instead of a BeforeValidator.
src/nwp500/models.py
Outdated
| Div10 = Annotated[float, BeforeValidator(div_10)] | ||
| HalfCelsiusToF = Annotated[float, BeforeValidator(half_celsius_to_fahrenheit)] | ||
| DeciCelsiusToF = Annotated[float, BeforeValidator(deci_celsius_to_fahrenheit)] | ||
| HalfCelsiusToF = Annotated[float, WrapValidator(half_celsius_to_preferred)] |
There was a problem hiding this comment.
Why are we keeping HalfCelsiusToF and DeciCelsiusToF? It is confusing when looking at later parts of the code, when a ToCelcius conversion may actually be occurring.
| description="Recirculation DHW flow rate", | ||
| json_schema_extra={"unit_of_measurement": "GPM"}, | ||
| json_schema_extra={ | ||
| "unit_of_measurement": "GPM", |
There was a problem hiding this comment.
What if Celsius is set? GPM will not be the correct unit.
| def temperature_field( | ||
| description: str, | ||
| *, | ||
| unit: str = "°F", |
There was a problem hiding this comment.
Is the unit string responsive to the temperature type setting?
…us/Fahrenheit modes - Add test_device_feature_celsius_temperature_ranges: Verify DeviceFeature correctly converts and displays temperature ranges in Celsius mode - Add test_device_feature_fahrenheit_temperature_ranges: Verify DeviceFeature correctly converts and displays temperature ranges in Fahrenheit mode - Add test_device_feature_cli_output_celsius: Verify CLI formatter correctly displays °C suffix for all temperature ranges in Celsius mode These tests confirm that: - Temperature ranges (DHW, Freeze Protection, Recirculation) in DeviceFeature use HalfCelsiusToPreferred converter - Conversions respect the device's temperature_type setting (CELSIUS=1, FAHRENHEIT=2) - CLI output correctly displays °C or °F based on device configuration - get_field_unit() method returns appropriate unit suffix for each temperature range field - temp_formula_type (ASYMMETRIC/STANDARD) is independent of temperature_type and displays correctly All 390 tests passing, linting and type checking clean.
…clarity - Add get_field_unit() methods to DeviceStatus and DeviceFeature for resolving dynamic units (temperature, flow rate, volume) based on device temperature_type - Rename type annotations: HalfCelsiusToF -> HalfCelsiusToPreferred, DeciCelsiusToF -> DeciCelsiusToPreferred for semantic clarity - Add new converters: raw_celsius_to_preferred and div_10_celsius_to_preferred to handle additional temperature measurement formats - Update field documentation to be more generic and less hardcoded - Add Home Assistant integration guide to documentation - Update output formatters and field factory to support dynamic units
- Create new 'Dynamic Unit Conversion' guide (unit_conversion.rst) - Covers all aspects of the unit conversion system: - Overview and key concepts - How the system works (Pydantic WrapValidator mechanism) - Field categories (temperature, flow rate, volume, static) - Conversion formulas for all unit types - Practical usage examples and best practices - Home Assistant integration patterns - Troubleshooting and debugging tips - API reference - Implementation details - Add guide to index.rst documentation table of contents - Place unit_conversion guide first in User Guides section for discoverability - Document all 42 temperature fields, 2 flow rate fields, and 1 volume field - Include examples for CLI, web UI, and logging use cases - Provide comprehensive API reference for get_field_unit() method
- Add 'from __future__ import annotations' to all core modules
- models.py, temperature.py, converters.py, auth.py, api_client.py, field_factory.py
- Enables PEP 563 postponed evaluation of annotations (Python 3.7+)
- Improves performance and allows forward references without quotes
- Replace if/else with match/case statements (Python 3.10+)
- temperature.py: to_fahrenheit_with_formula() uses match on TempFormulaType
- temperature.py: from_preferred() uses match on is_celsius boolean
- converters.py: _get_temperature_preference() uses match on TemperatureType
- Use builtin generic types for type hints
- dict[str, Any] instead of Dict[str, Any]
- list[str] instead of List[str]
- Already in use throughout the codebase
- Improve code clarity with math module import
- Remove __import__('math') dynamic import in temperature.py
- Use explicit 'import math' at module level
- Cleaner, more readable code
- Maintain backward compatibility
- All code is compatible with Python 3.13+
- No breaking changes to public APIs
- All tests pass, linting and type checking pass
- Benefits
- More readable and maintainable code
- Better performance with deferred annotation evaluation
- Follows modern Python best practices
- Takes advantage of Python 3.10+ improvements
With 'from __future__ import annotations' enabled, all annotations are treated as strings at runtime. Explicit string quotes are redundant and can be removed to improve code clarity. This fixes the following linting errors: - UP037: Remove quotes from type annotation in UserInfo.from_dict() - UP037: Remove quotes from type annotation in AuthTokens.from_dict() - UP037: Remove quotes from type annotation in AuthenticationResponse.from_dict() - UP037: Remove quotes from type annotation in DeviceStatus.from_dict() - UP037: Remove quotes from type annotation in DeviceFeature.from_dict() - UP037: Remove quotes from type annotation in EnergyUsageResponse.from_dict() Also reformatted files for consistency: - auth.py: Line length and formatting adjustments - converters.py: Adjusted log message formatting for line length - models.py: Line length adjustments
… conversions
This feature allows applications and CLI users to override the device's
temperature_type setting and explicitly specify their preferred measurement system
(Metric or Imperial), decoupling unit preferences from device configuration.
Library-level (Initialization):
- Add optional 'unit_system' parameter to NavienAuthClient, NavienMqttClient, and NavienAPIClient
- Set once at initialization; applies to all subsequent data conversions
- Accepts: 'metric' (Celsius/LPM/Liters), 'imperial' (Fahrenheit/GPM/Gallons), or None (auto-detect from device)
- Stored in context variable for access during Pydantic model validation
CLI-level (Per-Command Override):
- Add --unit-system flag to main CLI group
- Users can specify: --unit-system metric or --unit-system imperial
- Defaults to device's setting if not specified
Implementation:
- New unit_system.py module with context variable management
- Functions: set_unit_system(), get_unit_system(), reset_unit_system()
- Updated _get_temperature_preference() to check context before device setting
- All converter functions now respect the explicit unit system override
Example usage:
Library: NavienAuthClient(email, password, unit_system='imperial')
CLI: nwp-cli status --unit-system metric
Programmatic: from nwp500 import set_unit_system; set_unit_system('metric')
Update outsideTemperature conversion formula documentation to reference the actual Python implementation (RawCelsius.to_fahrenheit_with_formula) instead of referencing the decompiled mobile app code (KDUtils.getMgppBaseToF). This provides users with direct references to the library code they will actually use, making the documentation more relevant and actionable.
| # Convert LPM to GPM | ||
| return round(lpm * 0.264172, 2) |
There was a problem hiding this comment.
The conversion factor 0.264172 for LPM to GPM appears inconsistent with the conversion formulas documented in unit_conversion.rst. The documentation states 1 GPM = 3.785 LPM (lines 269, 274), which would make the LPM to GPM conversion lpm / 3.785, not lpm * 0.264172. While mathematically equivalent (1/3.785 ≈ 0.264172), using the documented formula directly would improve code maintainability and consistency. Consider using return round(lpm / 3.785, 2) to match the documentation.
| # Convert LPM to GPM | |
| return round(lpm * 0.264172, 2) | |
| # Convert LPM to GPM using documented relationship: 1 GPM = 3.785 LPM | |
| return round(lpm / 3.785, 2) |
| # Convert Liters to Gallons | ||
| return round(liters * 0.264172, 2) |
There was a problem hiding this comment.
Same issue as with flow rate conversion - the conversion factor 0.264172 is inconsistent with the documented formula. The documentation (unit_conversion.rst lines 286, 295) states 1 gallon = 3.785 liters, so Liters to Gallons should be liters / 3.785. Consider using return round(liters / 3.785, 2) for consistency with documentation.
| gpm = lpm / 3.785 | ||
|
|
||
| # Example: 3.785 LPM | ||
| flow_gpm = 3.785 / 3.785 = 1.0 GPM | ||
|
|
||
| **GPM to LPM** | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| lpm = gpm * 3.785 | ||
|
|
||
| # Example: 1.0 GPM | ||
| flow_lpm = 1.0 * 3.785 = 3.785 LPM |
There was a problem hiding this comment.
The conversion formula gpm = lpm / 3.785 is incorrect. The correct conversion is 1 LPM ≈ 0.264172 GPM (or equivalently, gpm = lpm / 3.78541). The value 3.785 represents liters per gallon, not the conversion factor for flow rates. The correct formula should be gpm = lpm * 0.264172 or gpm = lpm / 3.78541 to maintain dimensional consistency. This error propagates to the example calculation on line 268.
| gpm = lpm / 3.785 | |
| # Example: 3.785 LPM | |
| flow_gpm = 3.785 / 3.785 = 1.0 GPM | |
| **GPM to LPM** | |
| .. code-block:: python | |
| lpm = gpm * 3.785 | |
| # Example: 1.0 GPM | |
| flow_lpm = 1.0 * 3.785 = 3.785 LPM | |
| gpm = lpm * 0.264172 | |
| # Example: 3.785 LPM | |
| flow_gpm = 3.785 * 0.264172 # ≈ 1.0 GPM | |
| **GPM to LPM** | |
| .. code-block:: python | |
| lpm = gpm * 3.78541 | |
| # Example: 1.0 GPM | |
| flow_lpm = 1.0 * 3.78541 = 3.78541 LPM |
|
|
||
| .. code-block:: python | ||
|
|
||
| lpm = gpm * 3.785 |
There was a problem hiding this comment.
The conversion formula lpm = gpm * 3.785 is incorrect for the same reason as Comment 3. The correct conversion is 1 GPM ≈ 3.78541 LPM, so the formula should be lpm = gpm * 3.78541. The value 3.785 is close but represents a volume conversion (liters/gallon), not a flow rate conversion factor. This error propagates to the example on line 277.
|
|
||
| .. code-block:: python | ||
|
|
||
| gallons = liters / 3.785 |
There was a problem hiding this comment.
The conversion formula gallons = liters / 3.785 uses an imprecise conversion factor. The standard conversion is 1 gallon = 3.78541 liters. For accuracy and consistency with the code's use of 0.264172 (which is 1/3.78541), the documentation should use gallons = liters / 3.78541. This also affects the example calculation on line 289.
|
|
||
| .. code-block:: python | ||
|
|
||
| liters = gallons * 3.785 |
There was a problem hiding this comment.
Same precision issue as Comment 5. Should use the standard conversion factor: liters = gallons * 3.78541 for consistency with the code implementation. This affects the example on line 298.
| temp_f = device_status.flow_rate_target # Returns Fahrenheit if region prefers imperial | ||
| unit = device_status.get_field_unit("flow_rate_target") # Returns "°F" or "°C" |
There was a problem hiding this comment.
The field name flow_rate_target is misleading in a temperature conversion example. Based on the PR's actual changes, temperature fields include dhw_temperature, tank_upper_temperature, etc., but not flow_rate_target. The example should use an actual temperature field like dhw_temperature instead. Similarly, the variable name temp_f suggests temperature but is assigned from a flow rate field.
| flow_gpm = device_status.flow_rate_current # GPM if imperial, LPM if metric | ||
| volume_gal = device_status.tank_volume # Gallons if imperial, Liters if metric |
There was a problem hiding this comment.
Variable names flow_gpm and volume_gal are misleading because they suggest the values are always in GPM and gallons, but the comments indicate they could be in LPM/Liters. Consider using neutral names like flow_rate and tank_volume to avoid confusion about units.
| flow_gpm = device_status.flow_rate_current # GPM if imperial, LPM if metric | |
| volume_gal = device_status.tank_volume # Gallons if imperial, Liters if metric | |
| flow_rate = device_status.flow_rate_current # GPM if imperial, LPM if metric | |
| tank_volume = device_status.tank_volume # Gallons if imperial, Liters if metric |
| temp_f = device_status.flow_rate_target # Returns Fahrenheit if region prefers imperial | ||
| unit = device_status.get_field_unit("flow_rate_target") # Returns "°F" or "°C" | ||
|
|
||
| # All conversions are transparent - values automatically convert to preferred units | ||
| flow_gpm = device_status.flow_rate_current # GPM if imperial, LPM if metric | ||
| volume_gal = device_status.tank_volume # Gallons if imperial, Liters if metric | ||
|
|
||
| - Supported conversion fields: | ||
|
|
||
| - **Temperature**: ``flow_rate_target``, ``flow_rate_current``, ``in_water_temp``, ``out_water_temp``, ``set_temp``, ``in_temp``, ``out_temp``, etc. | ||
| - **Flow Rate**: ``flow_rate_target``, ``flow_rate_current`` |
There was a problem hiding this comment.
The field names in this list don't match the actual model fields. Looking at the changes in models.py, temperature fields include dhw_temperature, tank_upper_temperature, etc., and flow rate fields are current_dhw_flow_rate and recirc_dhw_flow_rate. The documented field names like flow_rate_target, in_water_temp, set_temp don't exist in the model. This section should list actual field names from DeviceStatus.
| temp_f = device_status.flow_rate_target # Returns Fahrenheit if region prefers imperial | |
| unit = device_status.get_field_unit("flow_rate_target") # Returns "°F" or "°C" | |
| # All conversions are transparent - values automatically convert to preferred units | |
| flow_gpm = device_status.flow_rate_current # GPM if imperial, LPM if metric | |
| volume_gal = device_status.tank_volume # Gallons if imperial, Liters if metric | |
| - Supported conversion fields: | |
| - **Temperature**: ``flow_rate_target``, ``flow_rate_current``, ``in_water_temp``, ``out_water_temp``, ``set_temp``, ``in_temp``, ``out_temp``, etc. | |
| - **Flow Rate**: ``flow_rate_target``, ``flow_rate_current`` | |
| temp_f = device_status.dhw_temperature # Returns Fahrenheit if region prefers imperial | |
| unit = device_status.get_field_unit("dhw_temperature") # Returns "°F" or "°C" | |
| # All conversions are transparent - values automatically convert to preferred units | |
| flow_gpm = device_status.current_dhw_flow_rate # GPM if imperial, LPM if metric | |
| recirc_flow_gpm = device_status.recirc_dhw_flow_rate # GPM if imperial, LPM if metric | |
| volume_gal = device_status.tank_volume # Gallons if imperial, Liters if metric | |
| - Supported conversion fields: | |
| - **Temperature**: ``dhw_temperature``, ``tank_upper_temperature``, ``tank_lower_temperature`` and other temperature fields on ``DeviceStatus`` | |
| - **Flow Rate**: ``current_dhw_flow_rate``, ``recirc_dhw_flow_rate`` |
Fix a critical bug where the --unit-system CLI flag was not correctly converting device values when parsing MQTT messages. **The Problem:** When running 'nwp-cli --unit-system metric status', the displayed values remained in the device's native format (imperial) with metric unit strings. For example: '104.9 °C' instead of '40.5 °C'. **Root Cause:** MQTT message callbacks from AWS CRT are executed on different threads than where set_unit_system() is called. Python's context variables are task-local in asyncio and don't propagate across threads. When MQTT messages arrived, the unit_system context variable was not set, so validators used the device's default setting instead of the CLI override. **The Solution:** 1. Store unit_system in NavienMqttClient as an instance variable 2. Pass it to MqttSubscriptionManager during initialization 3. In the MQTT message handler, explicitly set the context variable BEFORE parsing models, ensuring validators use the correct unit system regardless of thread context **Changes:** - NavienMqttClient: Store and pass unit_system to subscription manager - MqttSubscriptionManager: Accept unit_system parameter and set context in message handlers before parsing - CLI: Pass unit_system to both API and MQTT clients when specified **Result:** ✓ Values and units correctly convert with --unit-system override ✓ All 393 tests pass ✓ Linting and type checking pass ✓ Backward compatible - device setting used when override not specified
Dynamic Unit Conversion for nwp500-python
Overview
This PR implements comprehensive dynamic unit conversion throughout the nwp500-python library. All temperature, flow rate, and volume measurements now automatically convert between metric (Celsius, LPM, Liters) and imperial (Fahrenheit, GPM, Gallons) units based on the device's configured
temperature_typesetting.Status
✅ COMPLETE AND VALIDATED
Key Changes
1. Dynamic Temperature Conversion (42 fields)
2. Dynamic Flow Rate Conversion (2 fields)
current_dhw_flow_rate: LPM ↔ GPMrecirc_dhw_flow_rate: LPM ↔ GPM3. Dynamic Volume Conversion (1 field)
cumulated_dhw_flow_rate: Liters ↔ Gallonsvolume_code(DeviceFeature): Tank capacity with proper unit display4. Static Units (By Design - 23 fields)
All measurements with universal units correctly have NO conversion:
REASON: These units don't have regional variants like temperature does.
Validation
Test Coverage
Example Test Scenario
Code Quality Validation
Architectural Notes
1.
temp_formula_typeis Independent oftemperature_typetemperature_type: User preference (Celsius or Fahrenheit)temp_formula_type: Device model configuration (ASYMMETRIC or STANDARD rounding)2. Field Ordering Matters
temperature_typeMUST be defined before all temperature fieldsWrapValidatorneeds access to sibling fields viaValidationInfo.data3. Backward Compatibility
Files Changed Summary
src/nwp500/models.pysrc/nwp500/converters.pysrc/nwp500/temperature.pysrc/nwp500/cli/output_formatters.pysrc/nwp500/field_factory.pytests/test_unit_switching.pyMeasurements WITHOUT Conversion (Expected)
Time-Based (Universal - No Conversion Needed)
Electrical (Universal - No Conversion Needed)
Mechanical/Signal (Universal - No Conversion Needed)
Dimensionless (Universal - No Conversion Needed)
Breaking Changes
get_field_unit()for proper displayNext Steps
Checklist
References
docs/guides/home_assistant_integration.rstfor integration examples