From 67d1dde69fbacf33f2c39ea14d89f2afa425ed18 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Wed, 19 Oct 2022 13:31:08 +0200 Subject: [PATCH] Rename IMPERIAL_SYSTEM to US_CUSTOMARY_SYSTEM (#80253) * Rename IMPERIAL_SYSTEM * Deprecate is_metric property and adjust tests * Adjust unit_system config validation * Add yaml tests * Add tests for private name * Fix incorrect rebase * Adjust docstring * Add store migration * Update unit_system.py * Minimise test tweaks * Fix tests * Add conversion to migration * Rename new key and adjust tests * Adjust websocket_detect_config * Move original_unit_system tracking to subclass --- homeassistant/components/config/core.py | 2 +- homeassistant/core.py | 39 ++++++++++++++++-- homeassistant/util/unit_system.py | 28 ++++++++++--- tests/test_config.py | 53 +++++++++++++++++++++---- tests/util/test_unit_system.py | 25 +++++++++--- 5 files changed, 125 insertions(+), 22 deletions(-) diff --git a/homeassistant/components/config/core.py b/homeassistant/components/config/core.py index 5ffb2d4db99..c748395e95f 100644 --- a/homeassistant/components/config/core.py +++ b/homeassistant/components/config/core.py @@ -94,7 +94,7 @@ async def websocket_detect_config( info["unit_system"] = unit_system._CONF_UNIT_SYSTEM_METRIC else: # pylint: disable-next=protected-access - info["unit_system"] = unit_system._CONF_UNIT_SYSTEM_IMPERIAL + info["unit_system"] = unit_system._CONF_UNIT_SYSTEM_US_CUSTOMARY if location_info.latitude: info["latitude"] = location_info.latitude diff --git a/homeassistant/core.py b/homeassistant/core.py index 73fc18fd3b7..8f9287aedac 100644 --- a/homeassistant/core.py +++ b/homeassistant/core.py @@ -81,7 +81,13 @@ from .util.async_ import ( ) from .util.read_only_dict import ReadOnlyDict from .util.timeout import TimeoutManager -from .util.unit_system import METRIC_SYSTEM, UnitSystem, get_unit_system +from .util.unit_system import ( + _CONF_UNIT_SYSTEM_IMPERIAL, + _CONF_UNIT_SYSTEM_US_CUSTOMARY, + METRIC_SYSTEM, + UnitSystem, + get_unit_system, +) # Typing imports that create a circular dependency if TYPE_CHECKING: @@ -107,6 +113,7 @@ CALLBACK_TYPE = Callable[[], None] # pylint: disable=invalid-name CORE_STORAGE_KEY = "core.config" CORE_STORAGE_VERSION = 1 +CORE_STORAGE_MINOR_VERSION = 2 DOMAIN = "homeassistant" @@ -1986,7 +1993,7 @@ class Config: latitude=data.get("latitude"), longitude=data.get("longitude"), elevation=data.get("elevation"), - unit_system=data.get("unit_system"), + unit_system=data.get("unit_system_v2"), location_name=data.get("location_name"), time_zone=data.get("time_zone"), external_url=data.get("external_url", _UNDEF), @@ -2002,7 +2009,7 @@ class Config: "elevation": self.elevation, # We don't want any integrations to use the name of the unit system # so we are using the private attribute here - "unit_system": self.units._name, # pylint: disable=protected-access + "unit_system_v2": self.units._name, # pylint: disable=protected-access "location_name": self.location_name, "time_zone": self.time_zone, "external_url": self.external_url, @@ -2027,4 +2034,30 @@ class Config: CORE_STORAGE_KEY, private=True, atomic_writes=True, + minor_version=CORE_STORAGE_MINOR_VERSION, ) + self._original_unit_system: str | None = None # from old store 1.1 + + async def _async_migrate_func( + self, + old_major_version: int, + old_minor_version: int, + old_data: dict[str, Any], + ) -> dict[str, Any]: + """Migrate to the new version.""" + data = old_data + if old_major_version == 1 and old_minor_version < 2: + # In 1.2, we remove support for "imperial", replaced by "us_customary" + # Using a new key to allow rollback + self._original_unit_system = data.get("unit_system") + data["unit_system_v2"] = self._original_unit_system + if data["unit_system_v2"] == _CONF_UNIT_SYSTEM_IMPERIAL: + data["unit_system_v2"] = _CONF_UNIT_SYSTEM_US_CUSTOMARY + if old_major_version > 1: + raise NotImplementedError + return data + + async def async_save(self, data: dict[str, Any]) -> None: + if self._original_unit_system: + data["unit_system"] = self._original_unit_system + return await super().async_save(data) diff --git a/homeassistant/util/unit_system.py b/homeassistant/util/unit_system.py index 8dad108ccbe..31fec50c8f7 100644 --- a/homeassistant/util/unit_system.py +++ b/homeassistant/util/unit_system.py @@ -44,6 +44,7 @@ from .unit_conversion import ( _CONF_UNIT_SYSTEM_IMPERIAL: Final = "imperial" _CONF_UNIT_SYSTEM_METRIC: Final = "metric" +_CONF_UNIT_SYSTEM_US_CUSTOMARY: Final = "us_customary" LENGTH_UNITS = DistanceConverter.VALID_UNITS @@ -130,6 +131,9 @@ class UnitSystem: "Please adjust to use instance check instead.", error_if_core=False, ) + if self is IMPERIAL_SYSTEM: + # kept for compatibility reasons, with associated warning above + return _CONF_UNIT_SYSTEM_IMPERIAL return self._name @property @@ -213,15 +217,26 @@ class UnitSystem: def get_unit_system(key: str) -> UnitSystem: """Get unit system based on key.""" - if key == _CONF_UNIT_SYSTEM_IMPERIAL: - return IMPERIAL_SYSTEM + if key == _CONF_UNIT_SYSTEM_US_CUSTOMARY: + return US_CUSTOMARY_SYSTEM if key == _CONF_UNIT_SYSTEM_METRIC: return METRIC_SYSTEM raise ValueError(f"`{key}` is not a valid unit system key") +def _deprecated_unit_system(value: str) -> str: + """Convert deprecated unit system.""" + + if value == _CONF_UNIT_SYSTEM_IMPERIAL: + # need to add warning in 2023.1 + return _CONF_UNIT_SYSTEM_US_CUSTOMARY + return value + + validate_unit_system = vol.All( - vol.Lower, vol.Any(_CONF_UNIT_SYSTEM_METRIC, _CONF_UNIT_SYSTEM_IMPERIAL) + vol.Lower, + _deprecated_unit_system, + vol.Any(_CONF_UNIT_SYSTEM_METRIC, _CONF_UNIT_SYSTEM_US_CUSTOMARY), ) METRIC_SYSTEM = UnitSystem( @@ -235,8 +250,8 @@ METRIC_SYSTEM = UnitSystem( LENGTH_MILLIMETERS, ) -IMPERIAL_SYSTEM = UnitSystem( - _CONF_UNIT_SYSTEM_IMPERIAL, +US_CUSTOMARY_SYSTEM = UnitSystem( + _CONF_UNIT_SYSTEM_US_CUSTOMARY, TEMP_FAHRENHEIT, LENGTH_MILES, SPEED_MILES_PER_HOUR, @@ -245,3 +260,6 @@ IMPERIAL_SYSTEM = UnitSystem( PRESSURE_PSI, LENGTH_INCHES, ) + +IMPERIAL_SYSTEM = US_CUSTOMARY_SYSTEM +"""IMPERIAL_SYSTEM is deprecated. Please use US_CUSTOMARY_SYSTEM instead.""" diff --git a/tests/test_config.py b/tests/test_config.py index 15c5f84bc42..0a125d8f121 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -27,11 +27,17 @@ from homeassistant.const import ( CONF_UNIT_SYSTEM_METRIC, __version__, ) -from homeassistant.core import ConfigSource, HomeAssistantError +from homeassistant.core import ConfigSource, HomeAssistant, HomeAssistantError from homeassistant.helpers import config_validation as cv import homeassistant.helpers.check_config as check_config from homeassistant.helpers.entity import Entity from homeassistant.loader import async_get_integration +from homeassistant.util.unit_system import ( + _CONF_UNIT_SYSTEM_US_CUSTOMARY, + METRIC_SYSTEM, + US_CUSTOMARY_SYSTEM, + UnitSystem, +) from homeassistant.util.yaml import SECRET_YAML from tests.common import get_test_config_dir, patch_yaml_files @@ -439,7 +445,7 @@ async def test_loading_configuration_from_storage_with_yaml_only(hass, hass_stor assert hass.config.config_source is ConfigSource.STORAGE -async def test_updating_configuration(hass, hass_storage): +async def test_igration_and_updating_configuration(hass, hass_storage): """Test updating configuration stores the new configuration.""" core_data = { "data": { @@ -448,7 +454,7 @@ async def test_updating_configuration(hass, hass_storage): "location_name": "Home", "longitude": 13, "time_zone": "Europe/Copenhagen", - "unit_system": "metric", + "unit_system": "imperial", "external_url": "https://www.example.com", "internal_url": "http://example.local", "currency": "BTC", @@ -463,10 +469,14 @@ async def test_updating_configuration(hass, hass_storage): ) await hass.config.async_update(latitude=50, currency="USD") - new_core_data = copy.deepcopy(core_data) - new_core_data["data"]["latitude"] = 50 - new_core_data["data"]["currency"] = "USD" - assert hass_storage["core.config"] == new_core_data + expected_new_core_data = copy.deepcopy(core_data) + # From async_update above + expected_new_core_data["data"]["latitude"] = 50 + expected_new_core_data["data"]["currency"] = "USD" + # 1.1 -> 1.2 store migration with migrated unit system + expected_new_core_data["data"]["unit_system_v2"] = "us_customary" + expected_new_core_data["minor_version"] = 2 + assert hass_storage["core.config"] == expected_new_core_data assert hass.config.latitude == 50 assert hass.config.currency == "USD" @@ -593,6 +603,35 @@ async def test_loading_configuration_from_packages(hass): ) +@pytest.mark.parametrize( + "unit_system_name, expected_unit_system", + [ + (CONF_UNIT_SYSTEM_METRIC, METRIC_SYSTEM), + (CONF_UNIT_SYSTEM_IMPERIAL, US_CUSTOMARY_SYSTEM), + (_CONF_UNIT_SYSTEM_US_CUSTOMARY, US_CUSTOMARY_SYSTEM), + ], +) +async def test_loading_configuration_unit_system( + hass: HomeAssistant, unit_system_name: str, expected_unit_system: UnitSystem +) -> None: + """Test backward compatibility when loading core config.""" + await config_util.async_process_ha_core_config( + hass, + { + "latitude": 60, + "longitude": 50, + "elevation": 25, + "name": "Huis", + "unit_system": unit_system_name, + "time_zone": "America/New_York", + "external_url": "https://www.example.com", + "internal_url": "http://example.local", + }, + ) + + assert hass.config.units is expected_unit_system + + @patch("homeassistant.helpers.check_config.async_check_ha_config_file") async def test_check_ha_config_file_correct(mock_check, hass): """Check that restart propagates to stop.""" diff --git a/tests/util/test_unit_system.py b/tests/util/test_unit_system.py index 3019d6d0ff6..2e558a7bd5d 100644 --- a/tests/util/test_unit_system.py +++ b/tests/util/test_unit_system.py @@ -22,8 +22,10 @@ from homeassistant.exceptions import HomeAssistantError from homeassistant.util.unit_system import ( _CONF_UNIT_SYSTEM_IMPERIAL, _CONF_UNIT_SYSTEM_METRIC, + _CONF_UNIT_SYSTEM_US_CUSTOMARY, IMPERIAL_SYSTEM, METRIC_SYSTEM, + US_CUSTOMARY_SYSTEM, UnitSystem, get_unit_system, ) @@ -320,17 +322,26 @@ def test_is_metric( @pytest.mark.parametrize( - "unit_system, expected_name", + "unit_system, expected_name, expected_private_name", [ - (METRIC_SYSTEM, _CONF_UNIT_SYSTEM_METRIC), - (IMPERIAL_SYSTEM, _CONF_UNIT_SYSTEM_IMPERIAL), + (METRIC_SYSTEM, _CONF_UNIT_SYSTEM_METRIC, _CONF_UNIT_SYSTEM_METRIC), + (IMPERIAL_SYSTEM, _CONF_UNIT_SYSTEM_IMPERIAL, _CONF_UNIT_SYSTEM_US_CUSTOMARY), + ( + US_CUSTOMARY_SYSTEM, + _CONF_UNIT_SYSTEM_IMPERIAL, + _CONF_UNIT_SYSTEM_US_CUSTOMARY, + ), ], ) def test_deprecated_name( - caplog: pytest.LogCaptureFixture, unit_system: UnitSystem, expected_name: str + caplog: pytest.LogCaptureFixture, + unit_system: UnitSystem, + expected_name: str, + expected_private_name: str, ) -> None: """Test the name is deprecated.""" assert unit_system.name == expected_name + assert unit_system._name == expected_private_name assert ( "Detected code that accesses the `name` property of the unit system." in caplog.text @@ -341,7 +352,7 @@ def test_deprecated_name( "key, expected_system", [ (_CONF_UNIT_SYSTEM_METRIC, METRIC_SYSTEM), - (_CONF_UNIT_SYSTEM_IMPERIAL, IMPERIAL_SYSTEM), + (_CONF_UNIT_SYSTEM_US_CUSTOMARY, US_CUSTOMARY_SYSTEM), ], ) def test_get_unit_system(key: str, expected_system: UnitSystem) -> None: @@ -349,7 +360,9 @@ def test_get_unit_system(key: str, expected_system: UnitSystem) -> None: assert get_unit_system(key) is expected_system -@pytest.mark.parametrize("key", [None, "", "invalid_custom"]) +@pytest.mark.parametrize( + "key", [None, "", "invalid_custom", _CONF_UNIT_SYSTEM_IMPERIAL] +) def test_get_unit_system_invalid(key: str) -> None: """Test get_unit_system with an invalid key.""" with pytest.raises(ValueError, match=f"`{key}` is not a valid unit system key"):