#the `todo` integration now is always

1 messages · Page 1 of 1 (latest)

ripe cairn
#

all base platform integrations are now loaded by default, not sure if we have similar issues with any other platform.

mental socket
#

I think that makes sense. You add a todo list via add integration 👍

ripe cairn
mental socket
#

done

humble hawk
#

But as a side question, are there more cases like this like calendar

supple matrix
#

any other suggestions?
Revive my PR and not load all these integrations in by default. 😄

fleet stag
#

Which was your PR Artur?

#

Was it an alternative to the one loading all the base entity integrations?

#

For reference, the reason they're all loaded now is https://github.com/home-assistant/core/pull/164863, and the rationale for that one is that it should always be possible to edit automations which refer to a trigger/condition/action implemented by an integration which defines base entities

supple matrix
fleet stag
#

This delays loading of http views, websocket commands and the panel. We might only want to delay loading the panel.

supple matrix
#

Subclassing EntityComponent, etc.

fleet stag
#

Why? I don't see what's bad?

#

Do you have a better suggestion?

supple matrix
#

I don't think we subclass it anywhere else, integrations shouldn't really be overriding the core internals like that IMO

fleet stag
#

Why not? We want to register the frontend resources when the first platform is loaded.

ripe cairn
#

we should add some API to do it instead

supple matrix
#

yeah, it should register a callback instead

ripe cairn
#

btw, why do we need to do it both on async_setup_entry and async_setup_platform? Isn't the later one enough?

supple matrix
#

No, that's just for YAML based, config entries go through setup entry

fleet stag
#

Let's not over engineer this, there's no need to complicate this IMO

#

Especially if you Artur want to convince us to load triggers, components and services in some other way: if we now add some official API to listen to platforms being loaded, that will end up being dead code

ripe cairn
#

but if Artur does not convince us, this will be forever code 🙂

#

but I am ok with keeping it for now, so we can fix it before release

supple matrix
#

it's fine to fix this before release, just add "ERIK ADDED THIS AND PROMISED TO CLEAN THIS UP IN THE NEAR FUTURE" comment

fleet stag
#

There's no need for an official API for such a minor thing as this. One of our base entity integrations can be considered part of core internals, and the subclass is not doing anything hacky, it just does something when the first platform is loaded.

supple matrix
#

That I disagree with strongly, we should not have such subclassing in our codebase.

fleet stag
#

Because?

#

The class is not marked as Final

#

I do understand the concerns that it's unexpected that integrations subclass EntityComponent. I don't want to add a promise there saying I will clean it up. But I'm OK to add a comment saying we should add some API to register to platform loaded events if more integrations need it.

#

FWIW, we have the EVENT_COMPONENT_LOADED event, but I think that only fires when integrations are loaded, not when individual platforms are loaded.

#

My PRs could be modified listen to that event and check if an integration caused the load of a calendar or todo platform, do you consider that cleaner than subclassing EntityComponent?

#

Maybe we can make use of async_process_integration_platforms instead

fleet stag
supple matrix
#

Yeah, that looks much better 👍

#

Make sure it works though

ripe cairn
#

I love that async_setup_platform is only for YAML, but async_process_integration_platforms is for both... 😒

fleet stag
#

For example, I think it will force loading an integration's todo/calendar platform even if the integration didn't intend to do that

supple matrix
#

Yeah, that's true

#

But it will just import the code, not actually run anything, right?

fleet stag
#

But it changes the semantics a bit. With this method it's "set up calendar frontend if at least one integration with calendar platform is loaded" (and same for todo)

#

It means we'll still load the panel a bit more often than in the previous release

supple matrix
#

I hate to say that and it's possible that I'm biased here, but I think my solution is actually the cleanest option, the only downside of it is that it doesn't handle actions currently

#

Which also wasn't the case before

fleet stag
#

yes yes, but since we want actions to be editable, not just triggers and conditions, your solution is disqualified. so it's not viable for this release

supple matrix
#

I mean they weren't for years, it is really that important to add that now?

fleet stag
#

To turn that around, is it a big deal if we load calendar or todo when loading integration with calendar or todo platforms also when they don't load those platforms?

#

Looking at the core integrations which exist today I don't think it's a problem. It would be annoying if something like template or mqtt gained a todo or calendar platform though.

supple matrix
#

I don't think it's a big problem in practice. But looking at async_process_integration_platforms it's really not meant to be used for entity platforms, which those are.

#

So as a temporary solution, more than OK, but we need something different long term.

fleet stag
#

yeah, but do you think it's better than my subclass? That one is very easy to reason about and has no weird side effects

#

The bad thing about the subclass is that it's unexpected that integrations subclass EntityComponent, but that could be solved by explaining in a comment why we do it.

supple matrix
#

Honestly hard for me to make judgment on this one. Could we maybe present all three options objectively in the core channel and let others give their opinions on it?

fleet stag
#

OK, I'll do it