ronantakizawa/codereview-bench huggingface dataset:
Repository: home-assistant/core
File: homeassistant/components/actron_air/entity.py (Python)

Code before:
"""Base entity classes for Actron Air integration."""

from actron_neo_api import ActronAirZone

from homeassistant.helpers.device_registry import DeviceInfo
from homeassistant.helpers.update_coordinator import CoordinatorEntity

from .const import DOMAIN
from .coordinator import ActronAirSystemCoordinator


class ActronAirAcEntity(CoordinatorEntity[ActronAirSystemCoordinator]):
    """Base class for Actron Air entities."""

    _attr_has_entity_name = True

    def __init__(self, coordinator: ActronAirSystemCoordinator) -> None:
        """Initialize the entity."""
        super().__init__(coordinator)
        self._serial_number = coordinator.serial_number

        self._attr_device_info: DeviceInfo = DeviceInfo(
            identifiers={(DOMAIN, self._serial_number)},
            name=coordinator.data.ac_system.system_name,
            manufacturer="Actron Air",
            model_id=coordinator.data.ac_system.master_wc_model,
            sw_version=coordinator.data.ac_system.master_wc_firmware_version,
            serial_number=self._serial_number,
        )

    @property
    def available(self) -> bool:
        """Return True if entity is available."""
        return not self.coordinator.is_device_stale()


class ActronAirZoneEntity(ActronAirAcEntity):

Review comment (refactor):
Would it make sense to have a base base, where the availability and the has_entity_name lives? Then you can simplify the climate.py inheritance a bit

Code after:
"""Base entity classes for Actron Air integration."""

from actron_neo_api import ActronAirZone

from homeassistant.helpers.device_registry import DeviceInfo
from homeassistant.helpers.update_coordinator import CoordinatorEntity

from .const import DOMAIN
from .coordinator import ActronAirSystemCoordinator


class ActronAirEntity(CoordinatorEntity[ActronAirSystemCoordinator]):
    """Base class for Actron Air entities."""

    _attr_has_entity_name = True

    def __init__(self, coordinator: ActronAirSystemCoordinator) -> None:
        """Initialize the entity."""
        super().__init__(coordinator)
        self._serial_number = coordinator.serial_number

    @property
    def available(self) -> bool:
        """Return True if entity is available."""
        return not self.coordinator.is_device_stale()


class ActronAirAcEntity(ActronAirEntity):
    """Base class for Actron Air entities."""

    def __init__(self, coordinator: ActronAirSystemCoordinator) -> None:
        """Initialize the entity."""
        super().__init__(coordinator)
        self._attr_device_info = DeviceInfo(
            identifiers={(DOMAIN, self._serial_number)},
            name=coordinator.data.ac_system.system_name,
            manufacturer="Actron Air",
Repository: home-assistant/core
File: homeassistant/components/sunricher_dali/sensor.py (Python)

Code before:
    _attr_name = "State"

    def __init__(self, device: Device) -> None:
        """Initialize the illuminance sensor."""
        super().__init__(device)
        self._device = device
        self._attr_native_value: float | None = None
        self._sensor_enabled: bool = True
        self._attr_device_info = DeviceInfo(
            identifiers={(DOMAIN, device.dev_id)},
            name=device.name,
            manufacturer=MANUFACTURER,
            model=device.model,
            via_device=(DOMAIN, device.gw_sn),
        )

    async def async_added_to_hass(self) -> None:
        """Handle entity addition to Home Assistant."""
        await super().async_added_to_hass()

        self.async_on_remove(
            self._device.register_listener(
                CallbackEventType.ILLUMINANCE_STATUS, self._handle_illuminance_status
            )
        )

        self.async_on_remove(
            self._device.register_listener(
                CallbackEventType.SENSOR_ON_OFF, self._handle_sensor_on_off
            )
        )

        self._device.read_status()

    @callback
    def _handle_illuminance_status(self, status: IlluminanceStatus) -> None:
        """Handle illuminance status updates."""
        illuminance_value = status["illuminance_value"]
        is_valid = status["is_valid"]

        if not is_valid:
            _LOGGER.debug(
                "%s %s value is not valid: %s lux",
                self._attr_name,
                self._attr_unique_id,
                illuminance_value,
            )
            return

        self._attr_native_value = illuminance_value
        self.schedule_update_ha_state()

Review comment (suggestion):
In this case I'd maybe overwrite native_value property and return `_attr_native_value` unless the sensor is disabled. This would prevent you from "enabling" the sensor when the other method gets called for some reason

Code after:
    def _handle_illuminance_status(self, status: IlluminanceStatus) -> None:
        """Handle illuminance status updates."""
        illuminance_value = status["illuminance_value"]
        is_valid = status["is_valid"]

        if not is_valid:
            _LOGGER.debug(
                "Illuminance value is not valid for device %s: %s lux",
                self._device.dev_id,
                illuminance_value,
            )
            return

        self._illuminance_value = illuminance_value
        self.schedule_update_ha_state()

    @callback
    def _handle_sensor_on_off(self, on_off: bool) -> None:
        """Handle sensor on/off updates."""
        self._sensor_enabled = on_off
        _LOGGER.debug(
            "Illuminance sensor enable state for device %s updated to: %s",
            self._device.dev_id,
            on_off,
        )
        self.schedule_update_ha_state()
Repository: home-assistant/core
File: homeassistant/components/pooldose/diagnostics.py (Python)

Code before:


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant, entry: PooldoseConfigEntry
) -> dict[str, Any]:
    """Return diagnostics for a config entry."""
    coordinator = entry.runtime_data

    return {
        "entry": {
            "title": entry.title,
            "unique_id": entry.unique_id,
            "version": entry.version,
        },
        "device_info": async_redact_data(coordinator.device_info, TO_REDACT),
        "coordinator": {
            "update_interval": (
                coordinator.update_interval.total_seconds()
                if coordinator.update_interval
                else None
            ),
            "last_update_success": coordinator.last_update_success,
            "last_exception": str(coordinator.last_exception)
            if coordinator.last_exception
            else None,
        },
        "data": coordinator.data,
    }


Review comment (suggestion):
In theory
1. update interval is static
2. last update success would also be visible from the coordinator data, if that's not there then it's False
3. last exception is already logged with way more info

Code after:
"""Diagnostics support for Pooldose."""

from __future__ import annotations

from typing import Any

from homeassistant.components.diagnostics import async_redact_data
from homeassistant.core import HomeAssistant

from . import PooldoseConfigEntry

TO_REDACT = {
    "IP",
    "MAC",
    "WIFI_SSID",
    "AP_SSID",
    "SERIAL_NUMBER",
    "DEVICE_ID",
    "OWNERID",
    "NAME",
    "GROUPNAME",
}


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant, entry: PooldoseConfigEntry
) -> dict[str, Any]:
    """Return diagnostics for a config entry."""
    coordinator = entry.runtime_data

    return {
        "device_info": async_redact_data(coordinator.device_info, TO_REDACT),
        "data": coordinator.data,
    }
Repository: home-assistant/core
File: homeassistant/components/hikvision/binary_sensor.py (Python)

Code before:

    @property
    def is_on(self) -> bool:
        """Return true if sensor is on."""
        return self._get_sensor_attributes()[0]

    @property
    def extra_state_attributes(self) -> dict[str, Any]:
        """Return the state attributes."""
        attrs = self._get_sensor_attributes()
        return {ATTR_LAST_TRIP_TIME: attrs[3]}

    async def async_added_to_hass(self) -> None:
        """Register callback when entity is added."""
        await super().async_added_to_hass()

        # Register callback with pyhik
        self._camera.add_update_callback(self._update_callback, self._callback_id)

    def _update_callback(self, msg: str) -> None:
        """Update the sensor's state when callback is triggered.

        This is called from pyhik's event stream thread, so we must schedule
        the state update to run in the event loop thread.
        """
        self.hass.loop.call_soon_threadsafe(self.async_write_ha_state)


Review comment (suggestion):
You can use the entity function schedule_update_ha_state that works threadsafe here. 

Code after:

    @property
    def is_on(self) -> bool:
        """Return true if sensor is on."""
        return self._get_sensor_attributes()[0]

    @property
    def extra_state_attributes(self) -> dict[str, Any]:
        """Return the state attributes."""
        attrs = self._get_sensor_attributes()
        return {ATTR_LAST_TRIP_TIME: attrs[3]}

    async def async_added_to_hass(self) -> None:
        """Register callback when entity is added."""
        await super().async_added_to_hass()

        # Register callback with pyhik
        self._camera.add_update_callback(self._update_callback, self._callback_id)

    def _update_callback(self, msg: str) -> None:
        """Update the sensor's state when callback is triggered.

        This is called from pyhik's event stream thread, so we use
        schedule_update_ha_state which is thread-safe.
        """
        self.schedule_update_ha_state()
Repository: home-assistant/core
File: homeassistant/components/analytics/__init__.py (Python)

Code before:

CONF_SNAPSHOTS_URL = "snapshots_url"

CONFIG_SCHEMA = vol.Schema(
    {
        DOMAIN: vol.Schema(
            {
                vol.Optional(CONF_SNAPSHOTS_URL): vol.Any(str, None),
            }
        )
    },
    extra=vol.ALLOW_EXTRA,
)

DATA_COMPONENT: HassKey[Analytics] = HassKey(DOMAIN)

LABS_SNAPSHOT_FEATURE = "snapshots"


async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
    """Set up the analytics integration."""
    analytics_config = config.get(DOMAIN, {})

    if CONF_SNAPSHOTS_URL in analytics_config:
        await labs.async_update_preview_feature(
            hass, DOMAIN, LABS_SNAPSHOT_FEATURE, True
        )
        snapshots_url = analytics_config[CONF_SNAPSHOTS_URL]
    else:
        snapshots_url = None

    analytics = Analytics(hass, snapshots_url)

    # Load stored data
    await analytics.load()

    started = False

    async def _async_handle_labs_update(
        event: Event[labs.EventLabsUpdatedData],
    ) -> None:
        """Handle labs feature toggle."""
        await analytics.save_preferences({ATTR_SNAPSHOTS: event.data["enabled"]})
        if started:
            await analytics.async_schedule()

    @callback
    def _async_labs_event_filter(event_data: labs.EventLabsUpdatedData) -> bool:
        """Filter labs events for this integration's snapshot feature."""
        return (
            event_data["domain"] == DOMAIN

Review comment (suggestion):
It's hard to know what boolean parameters mean without the keyword, unless you know the signature by heart.
```suggestion
            hass, DOMAIN, LABS_SNAPSHOT_FEATURE, enabled=True
```

Code after:

CONF_SNAPSHOTS_URL = "snapshots_url"

CONFIG_SCHEMA = vol.Schema(
    {
        DOMAIN: vol.Schema(
            {
                vol.Optional(CONF_SNAPSHOTS_URL): vol.Any(str, None),
            }
        )
    },
    extra=vol.ALLOW_EXTRA,
)

DATA_COMPONENT: HassKey[Analytics] = HassKey(DOMAIN)

LABS_SNAPSHOT_FEATURE = "snapshots"


async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
    """Set up the analytics integration."""
    analytics_config = config.get(DOMAIN, {})

    if CONF_SNAPSHOTS_URL in analytics_config:
        await labs.async_update_preview_feature(
            hass, DOMAIN, LABS_SNAPSHOT_FEATURE, enabled=True
        )
        snapshots_url = analytics_config[CONF_SNAPSHOTS_URL]
    else:
        snapshots_url = None

    analytics = Analytics(hass, snapshots_url)

    # Load stored data
    await analytics.load()

    started = False

    async def _async_handle_labs_update(
        event: Event[labs.EventLabsUpdatedData],
    ) -> None:
        """Handle labs feature toggle."""
        await analytics.save_preferences({ATTR_SNAPSHOTS: event.data["enabled"]})
        if started:
            await analytics.async_schedule()

    @callback
    def _async_labs_event_filter(event_data: labs.EventLabsUpdatedData) -> bool:
        """Filter labs events for this integration's snapshot feature."""
        return (
            event_data["domain"] == DOMAIN
Repository: home-assistant/core
File: homeassistant/components/viaggiatreno/sensor.py (Python)

Code before:
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType, StateType

_LOGGER = logging.getLogger(__name__)

ICON = "mdi:train"
MONITORED_INFO = [  # Backward compatibility with older versions
    "categoria",
    "compOrarioArrivoZeroEffettivo",
    "compOrarioPartenzaZeroEffettivo",
    "destinazione",
    "numeroTreno",
    "orarioArrivo",
    "orarioPartenza",
    "origine",
    "subTitle",
]

DEFAULT_NAME = "Train {}"

CONF_NAME = "train_name"
CONF_STATION_ID = "station_id"
CONF_TRAIN_ID = "train_id"

ARRIVED_STRING = "Arrived"
CANCELLED_STRING = "Cancelled"
NOT_DEPARTED_STRING = "Not yet departed"
NO_INFORMATION_STRING = "No information for this train now"

SCAN_INTERVAL = timedelta(minutes=2)

PLATFORM_SCHEMA = SENSOR_PLATFORM_SCHEMA.extend(
    {
        vol.Required(CONF_TRAIN_ID): cv.string,
        vol.Required(CONF_STATION_ID): cv.string,
        vol.Optional(CONF_NAME): cv.string,
    }
)


async def async_setup_platform(
    hass: HomeAssistant,
    config: ConfigType,
    async_add_entities: AddEntitiesCallback,
    discovery_info: DiscoveryInfoType | None = None,
) -> None:
    """Set up the ViaggiaTreno platform."""
    train_id = str(config.get(CONF_TRAIN_ID))
    station_id = str(config.get(CONF_STATION_ID))
    if not (name := config.get(CONF_NAME)):
        name = DEFAULT_NAME.format(train_id)
    tl = TrainLine(train_id=train_id, starting_station=station_id)

Review comment (suggestion):
```suggestion
NOT_DEPARTED_STRING = "Not departed yet"
```

Code after:
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType, StateType

_LOGGER = logging.getLogger(__name__)

ICON = "mdi:train"
MONITORED_INFO = [  # Backward compatibility with older versions
    "categoria",
    "compOrarioArrivoZeroEffettivo",
    "compOrarioPartenzaZeroEffettivo",
    "destinazione",
    "numeroTreno",
    "orarioArrivo",
    "orarioPartenza",
    "origine",
    "subTitle",
]

DEFAULT_NAME = "Train {}"

CONF_NAME = "train_name"
CONF_STATION_ID = "station_id"
CONF_TRAIN_ID = "train_id"

ARRIVED_STRING = "Arrived"
CANCELLED_STRING = "Cancelled"
NOT_DEPARTED_STRING = "Not departed yet"
NO_INFORMATION_STRING = "No information for this train now"

SCAN_INTERVAL = timedelta(minutes=2)

PLATFORM_SCHEMA = SENSOR_PLATFORM_SCHEMA.extend(
    {
        vol.Required(CONF_TRAIN_ID): cv.string,
        vol.Required(CONF_STATION_ID): cv.string,
        vol.Optional(CONF_NAME): cv.string,
    }
)


async def async_setup_platform(
    hass: HomeAssistant,
    config: ConfigType,
    async_add_entities: AddEntitiesCallback,
    discovery_info: DiscoveryInfoType | None = None,
) -> None:
    """Set up the ViaggiaTreno platform."""
    train_id = str(config.get(CONF_TRAIN_ID))
    station_id = str(config.get(CONF_STATION_ID))
    if not (name := config.get(CONF_NAME)):
        name = DEFAULT_NAME.format(train_id)
    tl = TrainLine(train_id=train_id, starting_station=station_id)
Repository: home-assistant/core
File: tests/components/shopping_list/conftest.py (Python)

Code before:
"""Shopping list test helpers."""

from collections.abc import Generator
from contextlib import suppress
import os

import pytest

from homeassistant.components.shopping_list import intent as sl_intent
from homeassistant.core import HomeAssistant

from tests.common import MockConfigEntry


@pytest.fixture(autouse=True)
def wipe_shopping_list_store(hass: HomeAssistant) -> Generator[None]:
    """Wipe shopping list store after test."""
    try:
        yield
    finally:
        with suppress(FileNotFoundError):
            os.remove(hass.config.path(".shopping_list.json"))


@pytest.fixture
def mock_config_entry() -> MockConfigEntry:
    """Config Entry fixture."""
    return MockConfigEntry(domain="shopping_list")


@pytest.fixture
async def sl_setup(hass: HomeAssistant, mock_config_entry: MockConfigEntry) -> None:
    """Set up the shopping list."""

    mock_config_entry.add_to_hass(hass)

    assert await hass.config_entries.async_setup(mock_config_entry.entry_id)

    await sl_intent.async_setup_intents(hass)


Review comment (suggestion):
We should probably redesign shopping list to use storage properly, but in the meantime, this is a reasonable solution to ensure each test is properly isolated

Code after:
"""Shopping list test helpers."""

from collections.abc import Generator
from pathlib import Path
from unittest.mock import patch

import pytest

from homeassistant.components.shopping_list import PERSISTENCE, intent as sl_intent
from homeassistant.core import HomeAssistant

from tests.common import MockConfigEntry


@pytest.fixture(autouse=True)
def shopping_list_tmp_path(tmp_path: Path, hass: HomeAssistant) -> Generator[None]:
    """Use a unique temp directory for shopping list storage per test."""
    orig_path = hass.config.path

    def _mock_path(*args: str) -> str:
        if args == (PERSISTENCE,):
            return str(tmp_path / PERSISTENCE)
        return orig_path(*args)

    with patch.object(hass.config, "path", _mock_path):
        yield


@pytest.fixture
def mock_config_entry() -> MockConfigEntry:
    """Config Entry fixture."""
    return MockConfigEntry(domain="shopping_list")


@pytest.fixture
async def sl_setup(hass: HomeAssistant, mock_config_entry: MockConfigEntry) -> None:
    """Set up the shopping list."""

    mock_config_entry.add_to_hass(hass)

    assert await hass.config_entries.async_setup(mock_config_entry.entry_id)

    await sl_intent.async_setup_intents(hass)
Repository: home-assistant/core
File: tests/components/remote_calendar/test_config_flow.py (Python)

Code before:
        )
    )

    result2 = await hass.config_entries.flow.async_configure(
        result["flow_id"],
        user_input={
            CONF_CALENDAR_NAME: CALENDAR_NAME,
            CONF_URL: CALENDER_URL,
            CONF_VERIFY_SSL: True,
        },
    )

    assert result2["type"] is FlowResultType.FORM
    assert result2["errors"] == {"base": "invalid_ics_file"}
    respx.get(CALENDER_URL).mock(
        return_value=Response(
            status_code=200,
            text=ics_content,
        )
    )
    result3 = await hass.config_entries.flow.async_configure(
        result["flow_id"],
        {
            CONF_CALENDAR_NAME: CALENDAR_NAME,
            CONF_URL: CALENDER_URL,
            CONF_VERIFY_SSL: True,
        },
    )
    assert result3["type"] is FlowResultType.CREATE_ENTRY
    assert result3["title"] == CALENDAR_NAME
    assert result3["data"] == {
        CONF_CALENDAR_NAME: CALENDAR_NAME,
        CONF_URL: CALENDER_URL,
        CONF_VERIFY_SSL: True,
    }


async def test_duplicate_name(
    hass: HomeAssistant,
    config_entry: MockConfigEntry,
) -> None:
    """Test two calendars cannot be added with the same name."""

    await setup_integration(hass, config_entry)
    result = await hass.config_entries.flow.async_init(
        DOMAIN, context={"source": SOURCE_USER}
    )
    assert result["type"] is FlowResultType.FORM
    assert not result.get("errors")

    result2 = await hass.config_entries.flow.async_configure(

Review comment (suggestion):
```suggestion
            CONF_VERIFY_SSL: False,
```

Code after:
        )
    )

    result2 = await hass.config_entries.flow.async_configure(
        result["flow_id"],
        user_input={
            CONF_CALENDAR_NAME: CALENDAR_NAME,
            CONF_URL: CALENDER_URL,
            CONF_VERIFY_SSL: True,
        },
    )

    assert result2["type"] is FlowResultType.FORM
    assert result2["errors"] == {"base": "invalid_ics_file"}
    respx.get(CALENDER_URL).mock(
        return_value=Response(
            status_code=200,
            text=ics_content,
        )
    )
    result3 = await hass.config_entries.flow.async_configure(
        result["flow_id"],
        {
            CONF_CALENDAR_NAME: CALENDAR_NAME,
            CONF_URL: CALENDER_URL,
            CONF_VERIFY_SSL: False,
        },
    )
    assert result3["type"] is FlowResultType.CREATE_ENTRY
    assert result3["title"] == CALENDAR_NAME
    assert result3["data"] == {
        CONF_CALENDAR_NAME: CALENDAR_NAME,
        CONF_URL: CALENDER_URL,
        CONF_VERIFY_SSL: False,
    }


async def test_duplicate_name(
    hass: HomeAssistant,
    config_entry: MockConfigEntry,
) -> None:
    """Test two calendars cannot be added with the same name."""

    await setup_integration(hass, config_entry)
    result = await hass.config_entries.flow.async_init(
        DOMAIN, context={"source": SOURCE_USER}
    )
    assert result["type"] is FlowResultType.FORM
    assert not result.get("errors")

    result2 = await hass.config_entries.flow.async_configure(