Skip to content

Commit 246d018

Browse files
committed
Drop the coordinator's private copy of hass (P3-2)
DataUpdateCoordinator.__init__ already assigns self.hass, so `self._hass = hass` left every coordinator carrying two names for one object. They cannot diverge - the same hass is assigned and then handed to super() a few lines later - which is why this sat at P3 rather than higher. Worth removing anyway. The underscore claimed an ownership this code does not have: hass belongs to Home Assistant, which exposes it publicly and reads it in its own methods. While hass stays a plain attribute the two are identical; if it ever became a property, the private copy would quietly bypass whatever that property did. views.py and config_flow.py already use self.hass, so the coordinator was also the odd one out inside this repo. Both reads happen long after construction, and nothing before super() touches self.hass, so there is no ordering hazard. Seven tests reached the attribute through coordinator._hass. Confirmed they actually exercise it by making the source change first: all seven failed with AttributeError before being retargeted, so the rename is tested rather than assumed. Two new tests pin the base class's hass as the one it was given and refuse the private copy coming back. 361 tests, ruff clean, mypy clean against Home Assistant 2026.2.3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLPEGFSy3fLEuXNUPAPWR4
1 parent da54df4 commit 246d018

6 files changed

Lines changed: 37 additions & 10 deletions

File tree

custom_components/cellar_tracker/cellar_data.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,6 @@ class WineCellarData(DataUpdateCoordinator[CellarData]):
269269

270270
def __init__(self, hass: HomeAssistant, entry: ConfigEntry) -> None:
271271
"""Initialize the data coordinator."""
272-
self._hass = hass
273272
self._username = entry.data[CONF_USERNAME]
274273
self._password = entry.data[CONF_PASSWORD]
275274
self._currency = normalize_currency(
@@ -514,7 +513,7 @@ def _process_inventory(
514513
async def _fetch_payload(self) -> str:
515514
"""Fetch the raw inventory export for this entry's account."""
516515
return await async_fetch_inventory_payload(
517-
self._hass, self._username, self._password
516+
self.hass, self._username, self._password
518517
)
519518

520519
async def _async_update_data(self) -> CellarData:
@@ -546,7 +545,7 @@ async def _async_update_data(self) -> CellarData:
546545
# means splitting 66 columns per row, hashing each one and copying every
547546
# dict. self.data is the last successful result, or None on first poll.
548547
try:
549-
data = await self._hass.async_add_executor_job(
548+
data = await self.hass.async_add_executor_job(
550549
self._parse_and_process, payload, self.data
551550
)
552551
except UpdateFailed:

tests/test_async_fetch.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ def update(coordinator):
5454
def test_the_http_call_does_not_go_through_an_executor():
5555
coordinator = build(text=TWO_BOTTLES)
5656
update(coordinator)
57-
assert "get_inventory" not in coordinator._hass.executor_jobs, (
57+
assert "get_inventory" not in coordinator.hass.executor_jobs, (
5858
"the blocking client is still being used"
5959
)
6060

@@ -63,7 +63,7 @@ def test_parsing_still_runs_in_the_executor():
6363
"""CPU-bound work stays off the event loop even though I/O no longer needs it."""
6464
coordinator = build(text=TWO_BOTTLES)
6565
update(coordinator)
66-
assert coordinator._hass.executor_jobs == ["_parse_and_process"], (
66+
assert coordinator.hass.executor_jobs == ["_parse_and_process"], (
6767
"parsing must be the only executor job, and must still be one"
6868
)
6969

@@ -74,13 +74,13 @@ def test_parsing_still_runs_in_the_executor():
7474
def test_the_request_targets_the_library_endpoint():
7575
coordinator = build(text=TWO_BOTTLES)
7676
update(coordinator)
77-
assert coordinator._hass.session.requests[0]["url"] == BASE_URL
77+
assert coordinator.hass.session.requests[0]["url"] == BASE_URL
7878

7979

8080
def test_the_request_carries_the_expected_query():
8181
coordinator = build(text=TWO_BOTTLES)
8282
update(coordinator)
83-
params = coordinator._hass.session.requests[0]["params"]
83+
params = coordinator.hass.session.requests[0]["params"]
8484
assert params["User"] == "alice"
8585
assert params["Password"] == "s3cret"
8686
assert params["Table"] == "Inventory"

tests/test_coordinator_config_entry.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,31 @@ def test_the_entry_is_not_smuggled_through_kwargs():
3939
"""It has to be the named parameter, not an extra the base class ignores."""
4040
coordinator, _ = build()
4141
assert "config_entry" not in coordinator.init_kwargs
42+
43+
44+
# --------------------------------------------------------------------------
45+
# P3-2: no private copy of the base class's hass
46+
# --------------------------------------------------------------------------
47+
def test_the_base_class_hass_is_the_one_it_was_given():
48+
"""DataUpdateCoordinator.__init__ assigns it; nothing else needs to."""
49+
hass = FakeHass()
50+
hass.session = FakeSession()
51+
coordinator = WineCellarData(hass, ConfigEntry(entry_id="e", data=ENTRY_DATA))
52+
53+
assert coordinator.hass is hass
54+
55+
56+
def test_it_keeps_no_private_copy_of_hass():
57+
"""`_hass` shadowed `self.hass`, which the base class already provides.
58+
59+
Two names for one object, and the underscore claimed an ownership this
60+
code does not have: `hass` belongs to Home Assistant, which exposes it
61+
publicly and reads it in its own methods. Harmless while `hass` stays a
62+
plain attribute - if it ever became a property, the private copy would
63+
quietly bypass whatever that property did.
64+
"""
65+
coordinator, _ = build()
66+
67+
assert not hasattr(coordinator, "_hass"), (
68+
"self._hass duplicates the base class's self.hass; use that instead"
69+
)

tests/test_inventory_performance.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ def test_totals_are_unaffected_by_the_refactor():
120120
def test_parsing_is_handed_to_the_executor():
121121
coordinator = build_coordinator(returns=identical_bottles(10))
122122
asyncio.run(coordinator._async_update_data())
123-
assert "_parse_and_process" in coordinator._hass.executor_jobs, (
123+
assert "_parse_and_process" in coordinator.hass.executor_jobs, (
124124
"parsing ran on the event loop"
125125
)
126126

tests/test_parse_error_taxonomy.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def test_a_deliberate_update_failure_is_not_rewrapped():
7070
assert coordinator.data["total_bottles"] == 1
7171

7272
# An empty response after the cellar held stock: rejected on the first poll.
73-
coordinator._hass.session = FakeSession(text=HEADER)
73+
coordinator.hass.session = FakeSession(text=HEADER)
7474
with pytest.raises(UpdateFailed) as caught:
7575
asyncio.run(coordinator._async_update_data())
7676

tests/test_rate_limit.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ def test_the_interval_is_restored_after_a_successful_poll():
9494
refresh(coordinator)
9595
assert coordinator.update_interval == timedelta(seconds=1800)
9696

97-
coordinator._hass.session = FakeSession(text=GOOD)
97+
coordinator.hass.session = FakeSession(text=GOOD)
9898
asyncio.run(coordinator._async_update_data())
9999

100100
assert coordinator.update_interval == timedelta(seconds=SCAN_INTERVAL)

0 commit comments

Comments
 (0)