From 2dec7136c8b6a40709830679c544e6e47093e6c5 Mon Sep 17 00:00:00 2001 From: Shai Ungar Date: Sat, 13 Jul 2024 11:54:27 +0300 Subject: [PATCH] Address post merge review on israel rail (#121872) * Address late israel rail review * transfers => trains --- .../components/israel_rail/coordinator.py | 30 +- .../components/israel_rail/icons.json | 6 +- .../components/israel_rail/sensor.py | 13 +- .../components/israel_rail/strings.json | 4 +- tests/components/israel_rail/conftest.py | 48 --- .../israel_rail/snapshots/test_sensor.ambr | 280 ++++++++++++++++-- tests/components/israel_rail/test_sensor.py | 27 +- 7 files changed, 275 insertions(+), 133 deletions(-) diff --git a/homeassistant/components/israel_rail/coordinator.py b/homeassistant/components/israel_rail/coordinator.py index 952a3923119..d707f8c5ea6 100644 --- a/homeassistant/components/israel_rail/coordinator.py +++ b/homeassistant/components/israel_rail/coordinator.py @@ -3,7 +3,7 @@ from __future__ import annotations from dataclasses import dataclass -from datetime import datetime, timedelta +from datetime import datetime import logging from israelrailapi import TrainSchedule @@ -25,22 +25,11 @@ class DataConnection: """A connection data class.""" departure: datetime | None - duration: int | None platform: str - remaining_time: str start: str destination: str train_number: str - transfers: int - - -def calculate_duration_in_seconds(start_time: str, end_time: str) -> int | None: - """Transform and calculate the duration from start and end time into seconds.""" - end_time_date = dt_util.parse_datetime(end_time) - start_time_date = dt_util.parse_datetime(start_time) - if not end_time_date or not start_time_date: - return None - return (end_time_date - start_time_date).seconds + trains: int def departure_time(train_route: TrainRoute) -> datetime | None: @@ -49,15 +38,6 @@ def departure_time(train_route: TrainRoute) -> datetime | None: return start_datetime.astimezone() if start_datetime else None -def remaining_time(departure) -> timedelta | None: - """Calculate the remaining time for the departure.""" - departure_datetime = dt_util.parse_datetime(departure) - - if departure_datetime: - return dt_util.as_local(departure_datetime) - dt_util.as_local(dt_util.utcnow()) - return None - - class IsraelRailDataUpdateCoordinator(DataUpdateCoordinator[list[DataConnection]]): """A IsraelRail Data Update Coordinator.""" @@ -100,13 +80,9 @@ class IsraelRailDataUpdateCoordinator(DataUpdateCoordinator[list[DataConnection] departure=departure_time(train_routes[i]), train_number=train_routes[i].trains[0].data["trainNumber"], platform=train_routes[i].trains[0].platform, - transfers=len(train_routes[i].trains) - 1, - duration=calculate_duration_in_seconds( - train_routes[i].start_time, train_routes[i].end_time - ), + trains=len(train_routes[i].trains), start=station_name_to_id(train_routes[i].trains[0].src), destination=station_name_to_id(train_routes[i].trains[-1].dst), - remaining_time=str(remaining_time(train_routes[i].trains[0].departure)), ) for i in range(DEPARTURES_COUNT) if len(train_routes) > i and train_routes[i] is not None diff --git a/homeassistant/components/israel_rail/icons.json b/homeassistant/components/israel_rail/icons.json index c14e8804b98..39f8f24c77b 100644 --- a/homeassistant/components/israel_rail/icons.json +++ b/homeassistant/components/israel_rail/icons.json @@ -13,14 +13,14 @@ "duration": { "default": "mdi:timeline-clock" }, - "transfers": { - "default": "mdi:transit-transfer" + "trains": { + "default": "mdi:train" }, "platform": { "default": "mdi:bus-stop-uncovered" }, "train_number": { - "default": "mdi:train" + "default": "mdi:numeric" } } } diff --git a/homeassistant/components/israel_rail/sensor.py b/homeassistant/components/israel_rail/sensor.py index 1f6f20f82b2..132a9a74826 100644 --- a/homeassistant/components/israel_rail/sensor.py +++ b/homeassistant/components/israel_rail/sensor.py @@ -13,7 +13,6 @@ from homeassistant.components.sensor import ( SensorEntity, SensorEntityDescription, ) -from homeassistant.const import UnitOfTime from homeassistant.core import HomeAssistant from homeassistant.helpers.device_registry import DeviceEntryType, DeviceInfo from homeassistant.helpers.entity_platform import AddEntitiesCallback @@ -50,21 +49,15 @@ DEPARTURE_SENSORS: tuple[IsraelRailSensorEntityDescription, ...] = ( ) SENSORS: tuple[IsraelRailSensorEntityDescription, ...] = ( - IsraelRailSensorEntityDescription( - key="duration", - device_class=SensorDeviceClass.DURATION, - native_unit_of_measurement=UnitOfTime.SECONDS, - value_fn=lambda data_connection: data_connection.duration, - ), IsraelRailSensorEntityDescription( key="platform", translation_key="platform", value_fn=lambda data_connection: data_connection.platform, ), IsraelRailSensorEntityDescription( - key="transfers", - translation_key="transfers", - value_fn=lambda data_connection: data_connection.transfers, + key="trains", + translation_key="trains", + value_fn=lambda data_connection: data_connection.trains, ), IsraelRailSensorEntityDescription( key="train_number", diff --git a/homeassistant/components/israel_rail/strings.json b/homeassistant/components/israel_rail/strings.json index 48a7058de4a..f42cf765e22 100644 --- a/homeassistant/components/israel_rail/strings.json +++ b/homeassistant/components/israel_rail/strings.json @@ -28,8 +28,8 @@ "departure2": { "name": "Departure +2" }, - "transfers": { - "name": "Transfers" + "trains": { + "name": "Trains" }, "platform": { "name": "Platform" diff --git a/tests/components/israel_rail/conftest.py b/tests/components/israel_rail/conftest.py index 78abb0ee2f8..ba90cebe0a0 100644 --- a/tests/components/israel_rail/conftest.py +++ b/tests/components/israel_rail/conftest.py @@ -135,51 +135,3 @@ TRAINS = [ destination_station="3700", ), ] - -TRAINS_WRONG_FORMAT = [ - get_train_route( - train_number="1234", - departure_time="2021-10-1010:10:10", - arrival_time="2021-10-10T10:30:10", - origin_platform="1", - dest_platform="2", - origin_station="3500", - destination_station="3700", - ), - get_train_route( - train_number="1235", - departure_time="2021-10-1010:20:10", - arrival_time="2021-10-10T10:40:10", - origin_platform="1", - dest_platform="2", - origin_station="3500", - destination_station="3700", - ), - get_train_route( - train_number="1236", - departure_time="2021-10-1010:30:10", - arrival_time="2021-10-10T10:50:10", - origin_platform="1", - dest_platform="2", - origin_station="3500", - destination_station="3700", - ), - get_train_route( - train_number="1237", - departure_time="2021-10-1010:40:10", - arrival_time="2021-10-10T11:00:10", - origin_platform="1", - dest_platform="2", - origin_station="3500", - destination_station="3700", - ), - get_train_route( - train_number="1238", - departure_time="2021-10-1010:50:10", - arrival_time="2021-10-10T11:10:10", - origin_platform="1", - dest_platform="2", - origin_station="3500", - destination_station="3700", - ), -] diff --git a/tests/components/israel_rail/snapshots/test_sensor.ambr b/tests/components/israel_rail/snapshots/test_sensor.ambr index 8ad66cd970b..9806ecb1fae 100644 --- a/tests/components/israel_rail/snapshots/test_sensor.ambr +++ b/tests/components/israel_rail/snapshots/test_sensor.ambr @@ -143,7 +143,7 @@ 'state': '2021-10-10T10:30:10+00:00', }) # --- -# name: test_valid_config[sensor.mock_title_duration-entry] +# name: test_valid_config[sensor.mock_title_none-entry] EntityRegistryEntrySnapshot({ 'aliases': set({ }), @@ -155,7 +155,7 @@ 'disabled_by': None, 'domain': 'sensor', 'entity_category': None, - 'entity_id': 'sensor.mock_title_duration', + 'entity_id': 'sensor.mock_title_none', 'has_entity_name': True, 'hidden_by': None, 'icon': None, @@ -165,31 +165,123 @@ 'name': None, 'options': dict({ }), - 'original_device_class': , + 'original_device_class': None, 'original_icon': None, - 'original_name': 'Duration', + 'original_name': None, 'platform': 'israel_rail', 'previous_unique_id': None, 'supported_features': 0, - 'translation_key': None, - 'unique_id': 'באר יעקב אשקלון_duration', - 'unit_of_measurement': , + 'translation_key': 'platform', + 'unique_id': 'באר יעקב אשקלון_platform', + 'unit_of_measurement': None, }) # --- -# name: test_valid_config[sensor.mock_title_duration-state] +# name: test_valid_config[sensor.mock_title_none-state] StateSnapshot({ 'attributes': ReadOnlyDict({ 'attribution': 'Data provided by Israel rail.', - 'device_class': 'duration', - 'friendly_name': 'Mock Title Duration', - 'unit_of_measurement': , + 'friendly_name': 'Mock Title None', }), 'context': , - 'entity_id': 'sensor.mock_title_duration', + 'entity_id': 'sensor.mock_title_none', 'last_changed': , 'last_reported': , 'last_updated': , - 'state': '1200', + 'state': '1', + }) +# --- +# name: test_valid_config[sensor.mock_title_none_2-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'sensor', + 'entity_category': None, + 'entity_id': 'sensor.mock_title_none_2', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': None, + 'original_icon': None, + 'original_name': None, + 'platform': 'israel_rail', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'trains', + 'unique_id': 'באר יעקב אשקלון_trains', + 'unit_of_measurement': None, + }) +# --- +# name: test_valid_config[sensor.mock_title_none_2-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'attribution': 'Data provided by Israel rail.', + 'friendly_name': 'Mock Title None', + }), + 'context': , + 'entity_id': 'sensor.mock_title_none_2', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': '1', + }) +# --- +# name: test_valid_config[sensor.mock_title_none_3-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'sensor', + 'entity_category': None, + 'entity_id': 'sensor.mock_title_none_3', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': None, + 'original_icon': None, + 'original_name': None, + 'platform': 'israel_rail', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'train_number', + 'unique_id': 'באר יעקב אשקלון_train_number', + 'unit_of_measurement': None, + }) +# --- +# name: test_valid_config[sensor.mock_title_none_3-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'attribution': 'Data provided by Israel rail.', + 'friendly_name': 'Mock Title None', + }), + 'context': , + 'entity_id': 'sensor.mock_title_none_3', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': '1234', }) # --- # name: test_valid_config[sensor.mock_title_platform-entry] @@ -239,6 +331,150 @@ 'state': '1', }) # --- +# name: test_valid_config[sensor.mock_title_timestamp-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'sensor', + 'entity_category': None, + 'entity_id': 'sensor.mock_title_timestamp', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': , + 'original_icon': None, + 'original_name': 'Timestamp', + 'platform': 'israel_rail', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'departure0', + 'unique_id': 'באר יעקב אשקלון_departure', + 'unit_of_measurement': None, + }) +# --- +# name: test_valid_config[sensor.mock_title_timestamp-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'attribution': 'Data provided by Israel rail.', + 'device_class': 'timestamp', + 'friendly_name': 'Mock Title Timestamp', + }), + 'context': , + 'entity_id': 'sensor.mock_title_timestamp', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': '2021-10-10T10:10:10+00:00', + }) +# --- +# name: test_valid_config[sensor.mock_title_timestamp_2-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'sensor', + 'entity_category': None, + 'entity_id': 'sensor.mock_title_timestamp_2', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': , + 'original_icon': None, + 'original_name': 'Timestamp', + 'platform': 'israel_rail', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'departure1', + 'unique_id': 'באר יעקב אשקלון_departure1', + 'unit_of_measurement': None, + }) +# --- +# name: test_valid_config[sensor.mock_title_timestamp_2-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'attribution': 'Data provided by Israel rail.', + 'device_class': 'timestamp', + 'friendly_name': 'Mock Title Timestamp', + }), + 'context': , + 'entity_id': 'sensor.mock_title_timestamp_2', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': '2021-10-10T10:20:10+00:00', + }) +# --- +# name: test_valid_config[sensor.mock_title_timestamp_3-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'sensor', + 'entity_category': None, + 'entity_id': 'sensor.mock_title_timestamp_3', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': , + 'original_icon': None, + 'original_name': 'Timestamp', + 'platform': 'israel_rail', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'departure2', + 'unique_id': 'באר יעקב אשקלון_departure2', + 'unit_of_measurement': None, + }) +# --- +# name: test_valid_config[sensor.mock_title_timestamp_3-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'attribution': 'Data provided by Israel rail.', + 'device_class': 'timestamp', + 'friendly_name': 'Mock Title Timestamp', + }), + 'context': , + 'entity_id': 'sensor.mock_title_timestamp_3', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': '2021-10-10T10:30:10+00:00', + }) +# --- # name: test_valid_config[sensor.mock_title_train_number-entry] EntityRegistryEntrySnapshot({ 'aliases': set({ @@ -286,7 +522,7 @@ 'state': '1234', }) # --- -# name: test_valid_config[sensor.mock_title_transfers-entry] +# name: test_valid_config[sensor.mock_title_trains-entry] EntityRegistryEntrySnapshot({ 'aliases': set({ }), @@ -298,7 +534,7 @@ 'disabled_by': None, 'domain': 'sensor', 'entity_category': None, - 'entity_id': 'sensor.mock_title_transfers', + 'entity_id': 'sensor.mock_title_trains', 'has_entity_name': True, 'hidden_by': None, 'icon': None, @@ -310,26 +546,26 @@ }), 'original_device_class': None, 'original_icon': None, - 'original_name': 'Transfers', + 'original_name': 'Trains', 'platform': 'israel_rail', 'previous_unique_id': None, 'supported_features': 0, - 'translation_key': 'transfers', - 'unique_id': 'באר יעקב אשקלון_transfers', + 'translation_key': 'trains', + 'unique_id': 'באר יעקב אשקלון_trains', 'unit_of_measurement': None, }) # --- -# name: test_valid_config[sensor.mock_title_transfers-state] +# name: test_valid_config[sensor.mock_title_trains-state] StateSnapshot({ 'attributes': ReadOnlyDict({ 'attribution': 'Data provided by Israel rail.', - 'friendly_name': 'Mock Title Transfers', + 'friendly_name': 'Mock Title Trains', }), 'context': , - 'entity_id': 'sensor.mock_title_transfers', + 'entity_id': 'sensor.mock_title_trains', 'last_changed': , 'last_reported': , 'last_updated': , - 'state': '0', + 'state': '1', }) # --- diff --git a/tests/components/israel_rail/test_sensor.py b/tests/components/israel_rail/test_sensor.py index 8f338a80a86..d044dfe1d7c 100644 --- a/tests/components/israel_rail/test_sensor.py +++ b/tests/components/israel_rail/test_sensor.py @@ -12,7 +12,7 @@ from homeassistant.core import HomeAssistant from homeassistant.helpers import entity_registry as er from . import goto_future, init_integration -from .conftest import TRAINS, TRAINS_WRONG_FORMAT, get_time +from .conftest import TRAINS, get_time from tests.common import MockConfigEntry, snapshot_platform @@ -26,7 +26,7 @@ async def test_valid_config( ) -> None: """Ensure everything starts correctly.""" await init_integration(hass, mock_config_entry) - assert len(hass.states.async_entity_ids()) == 7 + assert len(hass.states.async_entity_ids()) == 6 await snapshot_platform(hass, entity_registry, snapshot, mock_config_entry.entry_id) @@ -38,7 +38,7 @@ async def test_update_train( ) -> None: """Ensure the train data is updated.""" await init_integration(hass, mock_config_entry) - assert len(hass.states.async_entity_ids()) == 7 + assert len(hass.states.async_entity_ids()) == 6 departure_sensor = hass.states.get("sensor.mock_title_departure") expected_time = get_time(10, 10) assert departure_sensor.state == expected_time @@ -47,27 +47,12 @@ async def test_update_train( await goto_future(hass, freezer) - assert len(hass.states.async_entity_ids()) == 7 + assert len(hass.states.async_entity_ids()) == 6 departure_sensor = hass.states.get("sensor.mock_title_departure") expected_time = get_time(10, 20) assert departure_sensor.state == expected_time -async def test_no_duration_wrong_date_format( - hass: HomeAssistant, - mock_israelrail: AsyncMock, - mock_config_entry: MockConfigEntry, -) -> None: - """Ensure the duration is not set when there is no departure time.""" - mock_israelrail.query.return_value = TRAINS_WRONG_FORMAT - await init_integration(hass, mock_config_entry) - assert len(hass.states.async_entity_ids()) == 7 - departure_sensor = hass.states.get("sensor.mock_title_train_number") - assert departure_sensor.state == "1234" - duration_sensor = hass.states.get("sensor.mock_title_duration") - assert duration_sensor.state == "unknown" - - async def test_fail_query( hass: HomeAssistant, freezer: FrozenDateTimeFactory, @@ -76,9 +61,9 @@ async def test_fail_query( ) -> None: """Ensure the integration handles query failures.""" await init_integration(hass, mock_config_entry) - assert len(hass.states.async_entity_ids()) == 7 + assert len(hass.states.async_entity_ids()) == 6 mock_israelrail.query.side_effect = Exception("error") await goto_future(hass, freezer) - assert len(hass.states.async_entity_ids()) == 7 + assert len(hass.states.async_entity_ids()) == 6 departure_sensor = hass.states.get("sensor.mock_title_departure") assert departure_sensor.state == STATE_UNAVAILABLE