Add HassDict implementation #103844
Add HassDict implementation #103844
Conversation
| @@ -103,17 +104,17 @@ async def async_setup_entry( | |||
| async_add_entities: AddEntitiesCallback, | |||
| ) -> None: | |||
| """Set up AdGuard Home sensor based on a config entry.""" | |||
| adguard = hass.data[DOMAIN][entry.entry_id][DATA_ADGUARD_CLIENT] | |||
| data = hass.data[ADGUARD_HASS_KEY][entry.entry_id] | |||
There was a problem hiding this comment.
The downside for me is that I can't just read the platform code anymore and know what the type is of data. As the type is different for each integration it's also not something you'll likely learn by heart compared to the core or helper interface functions. I'll have to jump around and check the type when reading the integration code.
There was a problem hiding this comment.
To be clear, this is what I'm comparing:
data: AdGuardData = hass.data[DOMAIN][entry.entry_id]There was a problem hiding this comment.
That's true, although I would argue it's manageable with good code completion. Also consider that the type (and any attributes) would get type checked and you're less likely to make an error as with type attributes on each individual platform. (Who says it's the correct type that's being assigned in sensor.py?)
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
This is still active. It just needs a decision if this approach should be explored further. |
|
I'm not a fan of this since it makes reading the code without extra tools than a browser a worse experience. I'd like to be able to see what type the key in |
Do you mean something like #96419? I ultimately didn't pursue this further as it would have required changes for basically every file that wants to use it. Additionally it didn't add much type safety as the types would only be evaluated inside the function context and not across the whole integration. Compare that to the current proposal where the type is set once per integration with |
|
I wouldn't make |
Unless I'm missing something, that's what I did there -> make
-- async def async_setup_entry(
hass: HomeAssistant,
entry: ConfigEntry,
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up Elgato button based on a config entry."""
coordinator: ElgatoDataUpdateCoordinator = hass.data[DOMAIN][entry.entry_id]
... |
|
Something like this, maybe. hass.data: hass.data[DOMAIN, ElgatoDataUpdateCoordinator]
coordinator = hass.data[DOMAIN][entry.entry_id]As a reader I don't need the |
Unfortunately, it's not possible to add annotations to attribute assignments. So you'd need to assign data: HassData[ElgatorDataUpdateCoordinator] = hass.data
coordinator = data[DOMAIN][entry.entry_id]IMO, this doesn't add much value over the current solution with coordinator: ElgatoDataUpdateCoordinator = hass.data[DOMAIN][entry.entry_id]--
I do understand your readability concerns, but please consider that it's only easy in some cases to understand the type currently. If it's a |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
This idea is now feature complete and fully working as intended with both Mypy and pyright (Pylance). I believe it represents a worthwhile improvement to our codebase and will help to catch issues with little additional effort.
I've added string compatibility for the HassKey types. I.e. for debugging we'd now get the nice dataclass
That is a valid concern and the only small downside IMO. As the data type is stored with the key itself, it would now be necessary to look it up first. This can be mitigated by code editors, e.g. for VSCode it's as easy as hovering over the variable to see its type. There is also an option to inlay the type itself: IMO it's wrong it compare it to the current approach of It also neglects the fact that often times The approach outlined in this PR has the big advantage that it's fully type checked across all uses. Not only for accessing a value but also for storing it. All the while only requiring one type annotation that needs to be updated if anything changes. |
|
It looks like VScode can work out the typing now 👍 That addresses my other concerns with this. 
If we can solve the run time performance issue, this makes sense to me. |
| @@ -738,7 +738,6 @@ async def test_integration_only_setup_entry(hass: HomeAssistant) -> None: | |||
| async def test_async_start_setup_running(hass: HomeAssistant) -> None: | |||
| """Test setup started context manager does nothing when running.""" | |||
| assert hass.state is CoreState.running | |||
| setup_started: dict[tuple[str, str | None], float] | |||
There was a problem hiding this comment.
I do really like how all this redundant code disappears
|
We've discussed this PR in the core team. We think the general idea is ok, but we'd like a specific solution for config entries first that stores the data on the config entry. We want it first since we expect a big influx of migration PRs for integrations when this is merged. The idea for config entries is to make the config entry generic and allow the integration to set what object type is stored. |
MartinHjelmare
removed
the
second-opinion-wanted
Thanks for the feedback! I tried to implement an MVP in #115669 just to get a feel for it. Also opened #115668 to get the unrelated AdGuard changes out of the way. Will rebase both PRs afterwards. I left some implementation comments on the other PR but overall I'm personally not fully convinced yet that it would be truly better to store the data inside the ConfigEntry. It somewhat seems to me like a solution in need of a problem. |
|
I've been testing Side note: The system health info isn't bound to an entry. So it seems we still need to keep |
|
Reverted the AdGuard changes here as we're going with the |
This comment was marked as spam.
This comment was marked as spam.
Proposed change
Alternative approach to typing
hass.data.For the previous attempt, see: #96419
Got the idea from
aiohttp. For3.9.0, they've addedAppKeyto solve a similar problem with typing theappdict.https://docs.aiohttp.org/en/latest/web_advanced.html#application-s-config
One of the reasons against the initial change was that ideally integrations shouldn't access
hass.datadirectly and the change was to involved for little value.Even though this diff might look bigger at first, it's actually easier to apply. Most of it are just the correct overloads. If at some point a good design for getter / setter emerges, this change can be easily incorporated or even fully reverted if necessary.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: