Use us_customary instead of imperial for Home Assistant consistency#65
Use us_customary instead of imperial for Home Assistant consistency#65
Conversation
Changed all references from 'imperial' to 'us_customary' to align with Home Assistant's official terminology. 'imperial' is maintained as a backward-compatible alias that maps to 'us_customary'. Changes: - set_unit_system() accepts 'us_customary', 'metric', or 'imperial' (deprecated) - get_unit_system() returns 'us_customary' or 'metric' - 'imperial' input automatically maps to 'us_customary' - All docstrings updated to reference 'us_customary' - Type hints support both for backward compatibility Rationale: Home Assistant uses 'us_customary' as the official term for the imperial measurement system. By using the same terminology, we ensure consistency and make integration with HA seamless. Backward Compatibility: - Existing code passing 'imperial' will continue to work - Internally converted to 'us_customary' for consistency - No breaking changes for users
There was a problem hiding this comment.
Pull request overview
This PR updates the codebase to use 'us_customary' instead of 'imperial' for unit system terminology, aligning with Home Assistant's official conventions while maintaining backward compatibility through automatic aliasing.
Changes:
- Modified
set_unit_system()to accept 'us_customary' as the primary term and automatically map 'imperial' to 'us_customary' for backward compatibility - Updated
get_unit_system()return type to exclude 'imperial' since it's now internally converted - Updated all docstrings, type hints, and comments across the codebase to reference 'us_customary' as the standard term
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/nwp500/unit_system.py | Core unit system logic updated with 'us_customary' terminology and imperial-to-us_customary mapping |
| src/nwp500/mqtt/subscriptions.py | Type hints updated to include 'us_customary' in MqttSubscriptionManager |
| src/nwp500/mqtt/client.py | Type hints and docstrings updated in MqttClient for 'us_customary' |
| src/nwp500/auth.py | Type hints and docstrings updated in NavienAuthClient for 'us_customary' |
| src/nwp500/api_client.py | Type hints and docstrings updated in NavienAPIClient for 'us_customary' |
Comments suppressed due to low confidence (1)
src/nwp500/unit_system.py:1
- The stored unit_system should not include 'imperial' since it's immediately converted to 'us_customary' in set_unit_system(). The type hint should be
Literal['metric', 'us_customary'] | Noneto reflect the internal state after conversion.
"""Unit system management for temperature, flow rate, and volume conversions.
src/nwp500/mqtt/client.py
Outdated
|
|
||
| self._auth_client = auth_client | ||
| self._unit_system: Literal["metric", "imperial"] | None = unit_system | ||
| self._unit_system: Literal["metric", "us_customary", "imperial"] | None = unit_system |
There was a problem hiding this comment.
The stored unit_system should not include 'imperial' in its type since set_unit_system() converts 'imperial' to 'us_customary'. The type hint should be Literal['metric', 'us_customary'] | None to reflect the actual stored values after conversion.
| self._unit_system: Literal["metric", "us_customary", "imperial"] | None = unit_system | |
| normalized_unit_system: Literal["metric", "us_customary"] | None | |
| if unit_system == "imperial": | |
| normalized_unit_system = "us_customary" | |
| else: | |
| normalized_unit_system = unit_system | |
| self._unit_system: Literal["metric", "us_customary"] | None = normalized_unit_system |
Since this hasn't been deployed yet, remove support for 'imperial' as input. Use only 'us_customary' consistently across the library. Changes: - set_unit_system() accepts only 'metric' or 'us_customary' (not 'imperial') - get_unit_system() returns 'metric' or 'us_customary' - Removed 'imperial' from all Literal type hints - Removed 'imperial' mapping logic - Updated all docstrings to reference only 'us_customary' - Updated unit_system_to_temperature_type to handle only 'us_customary' - Updated CLI to use 'us_customary' instead of 'imperial' - Updated all test references This ensures clean, consistent terminology aligned with Home Assistant's official 'us_customary' term.
- Fixed unit_system.py line 27: missing # on comment line - Added UnitSystemType type alias to mqtt/client.py and mqtt/subscriptions.py - Replaced long Literal type hints with UnitSystemType alias (fixes line length) - Fixed long docstring line in is_metric_preferred() return description All lines now within 80 character limit. Syntax errors resolved.
- Moved local imports before TYPE_CHECKING block (fixes E402 errors) - Split long docstring line in subscriptions.py (fixes E501 error) - All imports now properly ordered - All lines within 80 character limit
src/nwp500/mqtt/subscriptions.py
Outdated
| from ..unit_system import set_unit_system | ||
| from .utils import redact_topic, topic_matches_pattern | ||
|
|
||
| # Type alias for unit system preference | ||
| UnitSystemType = Literal["metric", "us_customary"] | None |
There was a problem hiding this comment.
The UnitSystemType alias is defined in two separate files (subscriptions.py and client.py). This duplication should be avoided by defining the type alias once in a shared location (e.g., unit_system.py where the unit system functionality is centralized) and importing it where needed.
| from ..unit_system import set_unit_system | |
| from .utils import redact_topic, topic_matches_pattern | |
| # Type alias for unit system preference | |
| UnitSystemType = Literal["metric", "us_customary"] | None | |
| from ..unit_system import UnitSystemType, set_unit_system | |
| from .utils import redact_topic, topic_matches_pattern |
src/nwp500/mqtt/client.py
Outdated
| # Type alias for unit system preference | ||
| UnitSystemType = Literal["metric", "us_customary"] | None |
There was a problem hiding this comment.
This is a duplicate definition of UnitSystemType that also appears in subscriptions.py. Define the type alias once in unit_system.py and import it in both locations to maintain a single source of truth.
- Defined UnitSystemType type alias in unit_system.py (single source of truth) - Removed duplicate definitions from mqtt/client.py and mqtt/subscriptions.py - Import UnitSystemType from unit_system module where needed This follows DRY principle and maintains type consistency across the codebase.
|
Regarding the suggestion about normalizing unit_system values in mqtt/client.py: Since we've now removed 'imperial' from the accepted values entirely (it's no longer in the UnitSystemType definition), the type system already prevents any 'imperial' values from being passed to init. The function signature: unit_system: UnitSystemType = Nonewhere ensures only valid values can be passed. Type checkers will catch any attempts to pass 'imperial' at static analysis time. However, I've addressed the DRY issue by consolidating the UnitSystemType alias to a single definition in unit_system.py and importing it in both mqtt/client.py and mqtt/subscriptions.py. This was a good catch! |
After consolidating UnitSystemType to unit_system.py, the Literal type is no longer directly used in subscriptions.py imports. Remove the unused import to fix pyright error.
- Removed unused Literal import from mqtt/client.py - Sorted imports alphabetically per ruff isort rules - UnitSystemType now comes before set_unit_system in imports - All ruff linting errors now resolved - Verified locally with tox -e lint before pushing
Overview
This PR removes all references to 'imperial' terminology and exclusively uses Home Assistant's official term 'us_customary' for measurement systems throughout the library.
Changes
Rationale
Since this version hasn't been deployed yet, clean up the API to exclusively use Home Assistant's official term 'us_customary' rather than maintaining backward compatibility with 'imperial'.
API Changes
Before:
After:
Backward Compatibility
None - this is a breaking change, but acceptable since it hasn't been released yet and makes the API cleaner and more aligned with Home Assistant standards.