Add additional type hints for calendar integration #68660
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( |
| @@ -97,7 +101,7 @@ def normalize_event(event): | |||
| return normalized_event | |||
|
|
|||
|
|
|||
| def calculate_offset(event, offset): | |||
| def calculate_offset(event: dict[str, Any], offset: str) -> dict[str, Any]: | |||
There was a problem hiding this comment.
We should type event correctly.
Although its technically an dict[str, Any], we do define its contents and should thus should type it.
IMHO, using loose typing like this is harmful (compared to no typing at all).
There was a problem hiding this comment.
Happy to update. I have some questions to help me understand what correct looks like here.
The set of fields used by calendar is hard to follow, so that makes sense.
-
To be a little more explicit: is that like TypedDict? Breaking change? Or feel free to point me at an example you like.
-
One issue: integrations today add whatever fields they like to this for example, the entire Google Calendar API response is in this. Is that an issue or do we want to specify every field in the API? Or break that?
-
Can you elaborate on why it's harmful? I'm not experienced enough with the down sides of this to have that context.
Thanks, I appreciate just a little more assistance then happy to resolve.
There was a problem hiding this comment.
The set of fields used by calendar is hard to follow, so that makes sense.
Hehe yeah, been playing with Calendar lately myself as well (added an implementation), but those fields are magical at this point (not typed, not documented). Which makes it hard to use and figure out.
To be a little more explicit: is that like TypedDict? Breaking change? Or feel free to point me at an example you like.
We should go for the best fit for this. Considering the low/limited use of this integration, I would not mind hard-breaking changes myself (to be honest). The impact would be low anyway.
I'm not entirely sure. TypeDicts could work, but they have quite a few disadvantages (especially around required/optional fields). Hence we mostly use dataclasses in Home Assistant right now.
One issue: integrations today add whatever fields they like to this for example, the entire Google Calendar API response is in this. Is that an issue or do we want to specify every field in the API? Or break that?
I don't this it's correct to have this event-free-for-all format. We could allow extra_attributes-a-like in an event (like we do with entities), but the base should be defined IMHO.
Can you elaborate on why it's harmful?
Not typing upstream, doesn't break when adding typing or making typing more strict. Adding typing everywhere and later change it, does require everything to be updated. Hence I think we should correctly type it from the get go (besides making it easier to use calendar stuff in the first place).
There was a problem hiding this comment.
Thanks that was helpful.
I agree the breaking change impact will be small as I think nothing or very little should be depending on this field. My impression is there are about 5 fields we actually would want to use in practice.
One consideration from the arch event trigger discussion: we were thinking about allowing integrations to pass arbitrary API fields in the event trigger payloads, to reduce what we need to put in the trigger schema. However maybe just having a fully specified Event schema is worth it.
Should this rise to the level of arch discussion to align on the event fields for calendar or do we just consider this cleanup given the low integration numbers for calendar?
There was a problem hiding this comment.
I don't think we need to do this change in this PR. It's a bigger change and better to do in a follow up.
There was a problem hiding this comment.
The status quo would be a typeddict I would say?
There was a problem hiding this comment.
I think the status quo includes arbitrary data from the Google API though, right? e.g. https://developers.google.com/calendar/api/v3/reference/events#resource via
-- unless it can ignore arbitrary extra fields. It looks like most of the other integrations are more uniform and use roughly the same fields.I'm happy to drive this cleanup as i've been cleaning up google already.
There was a problem hiding this comment.
The other related problem we have is that event, async_get_events, and the state attributes are all a little bit different in ways that are not particularly helpful at the moment.
There was a problem hiding this comment.
Slept over it, I think Martin and you are right, this is a step forward. And it still needs to be handled differently IMHO, but that might be not for now. All other things in this PR as still valuable. Making steps is important too
There was a problem hiding this comment.
I'll follow through on the clean up of stripping out unnecessary fields and adding in the typing. (So you won't regret the approval 🙂)
|
Sent #68724 to remove some dependencies on the event dict structure that currently add unnecessary fields. |
Proposed change
Add additional type hints for calendar integration as part of pre-work to implement event triggers (home-assistant/architecture#700)
Overall set of PRs for triggers:
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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: