Set min, max, and step for ViCare number entities #104593
Conversation
| @@ -10,7 +10,7 @@ | |||
| from PyViCare.PyViCareDevice import Device as PyViCareDevice | |||
| from PyViCare.PyViCareDeviceConfig import PyViCareDeviceConfig | |||
| from PyViCare.PyViCareHeatingDevice import ( | |||
| HeatingDeviceWithComponent as PyViCareHeatingDeviceWithComponent, | |||
| HeatingDeviceWithComponent as PyViCareHeatingDeviceComponent, | |||
There was a problem hiding this comment.
Whats the reason to rename the import? Why aren't we using the original name?
There was a problem hiding this comment.
To me this naming makes more sense. We deal here with burners, compressors or heating circuits which are components of a heating device and not heating devices as the name heating device with components suggest.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
This reverts commit e0a6c0e.
edenhaus
changed the title
| circuits = await hass.async_add_executor_job(get_circuits, api) | ||
| for circuit in circuits: | ||
| for description in CIRCUIT_ENTITY_DESCRIPTIONS: | ||
| entity = await hass.async_add_executor_job( |
There was a problem hiding this comment.
We should collect all the sync work in a function and schedule that work once on the executor. The context switch from event loop to the executor is expensive.
There was a problem hiding this comment.
Would it make sense to do that once in __init__ and store on hass.data[DOMAIN]?
There was a problem hiding this comment.
Entities are a platform concern. It would be ok to collect device information in a central place if it's relevant for more than one entity. But the entities themselves should not be stored anywhere outside their platform.
There was a problem hiding this comment.
Ah, I misunderstood, I thought this is about circuits = await hass.async_add_executor_job(get_circuits, api) which is done for all platforms.
There was a problem hiding this comment.
The issue is that the library caches the API data for 60 seconds and each access one of its objects can result in an update of that cache.
Probably an UpdateCoordinator would be better to have, but my knowledge neither in python nor in home assistant is deep enough to do that.
There was a problem hiding this comment.
I don't understand what the cache has to do with if we make one or multiple sequential executor jobs? It doesn't change how we access the library objects. It only changes how that work is scheduled from the event loop on the executor.
There was a problem hiding this comment.
Do you think about something like this where the async executor is moved outside the loops?
https://github.com/home-assistant/core/compare/dev...CFenner:sync-4?expand=1
There was a problem hiding this comment.
Yes, but use a list comprehension. We don't need any local variable for entity then.
Breaking change
Proposed change
With this the number entities get it's min, max and stepping values from the function definition of the api.
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: