#In my PR the mypy check fails on parts
1 messages ยท Page 1 of 1 (latest)
Looks like mypy suddenly found some issues with typing
For the Need type annotation for "_attributes" (hint: "_attributes: Dict[<type>, <type>] = ...") [var-annotated] ones, you should add a type to it
so instead of self._attributes = {} you should do self._attributes: dict[str, Any] = {}
For Incompatible types in assignment (expression has type "bool", variable has type "None") [assignment], you're initializing self._current_action with self._current_action = None which makes it with type None, you can change the initializiation to self._current_action: bool | None = None
No overload variant of "get" of "dict" matches argument type "None" [call-overload]: self._current_mode is currently typed None as that's how its initialized. You should type it as str
There are quite some issues, maybe it's worth it making a preliminary PR where you complete the typing to avoid these issues in the future
Looks like the check only validated changed files, right?
(as some issues can require more work, for example the call-overload one, you initialize it with None (iirc), and if you type it str | None, you have to make sure the code can handle if the variable is None)
yes
Also, just glanced quick over your PR. I would suggest splitting of the creation of the base entity to a separate PR
And I see a lot of other stuff happening in general
quite some changes are required by formatters which makes review a bit hard in my eyes.
Yep I can imagine
But like, also for reviewing the code itself, this PR is doing a lot imo
too much
Ok, then I'll try to split it.
Do you want me to suggest some steps you can make before this PR is easy to do?
Yes, please!
- Add typing. Start on dev and add typing to the function calls. I also see some stuff that can be moved outside of the constructor (for example, if you are doing
self._current_action = None, you can also move this outside of a constructor). This might require some change in logic as you now have to take care of nullables. - Introduce the base entity, preferably without
_attr_has_entity_name = True, as that usually leads to a bigger change. - add
_attr_has_entity_name = True - this PR
And it might look like some nitpicking, but this way you can focus on one thing at a time and the review process will go a lot quicker as we're not changing stuff all over the place
Thanks, I will have a look now.
Feel free to reach out if you have questions or want to discuss ๐
Is there a way to run the validation (pre-commit run --all-files ) just for this component?
And mypy fails locally as it's not found. Is this something I need to install or build?
eeuuuh
You can run ruff etc normally
I think there is a way to run pre-commit on a certain folder
make sure you have run script/setup
Can you maybe explain what you mean with multiple devices?
Here or on the PR?
oh nvm, I read the PR itself and it became clear
haha
I usually skip the whole PR and dive into the code, but that's just me
I just added another PR for the entity name: https://github.com/home-assistant/core/pull/101895/files
Ok.
Regarding the first PR, you would be fine without special device name handling and all the has_multiple_devices stuff?
I think so, but I'm also open do have a quick discussion with Robert to exchange toughts
Spoke him, we'll discuss this tomorrow
Am now discussing it with Frenck and Robert, we think showing no serial number in the name is the way to go
but to help with identifying the device, we think adding a serial number to the device_info
and show it in the frontend
This would be a new core feature
and we will work on the proposal and implementation of that
Ok, then I'll remove the serial completely for now. For me it works that way too as I have different models.
Nice work on the PRs btw ๐
@ionic hollow
btw, do you have a working dev environment?
No not really ๐
need help setting up?
Pytest seems not to work correct.
yes, thats working
I know next to nothing about python and how to setup a workspace. I've installed python via brew, but seems like mac has also it's own version.
pytest always complains about missing dependencies.
but how come you always have that much black and ruff stuff?
I sometimes do changes in GH directly
directly in vscode
with the command you gave me
pytest ./tests/components/vicare --snapshot-update
or only pytest
ah
the first one only runs vicare tests
and that one works completely right?
can you try pip -V
No, it gives
ImportError while loading conftest '/Users/.../private/home-assistant-core/tests/conftest.py'.
tests/conftest.py:36: in <module>
from . import patch_time # noqa: F401, isort:skip
tests/patch_time.py:7: in <module>
from homeassistant import runner, util
homeassistant/runner.py:17: in <module>
from . import bootstrap
homeassistant/bootstrap.py:19: in <module>
from . import config as conf_util, config_entries, core, loader
homeassistant/config.py:20: in <module>
from . import auth
homeassistant/auth/__init__.py:17: in <module>
from . import auth_store, jwt_wrapper, models
homeassistant/auth/jwt_wrapper.py:13: in <module>
from jwt import DecodeError, PyJWS, PyJWT
E ImportError: cannot import name 'DecodeError' from 'jwt' (/opt/homebrew/lib/python3.11/site-packages/jwt/__init__.py)
pip3 -V
pip 23.2.1 from /opt/homebrew/lib/python3.11/site-packages/pip (python 3.11)
pip3 install -r requirements.txt
pip3 install -r requirements_test.txt
try running those 2
Ah.. that helped! Is that not done via ./scrips/setup?
Ok, thanks for the help. And I will try to do changes only with pre-commit ๐
I mean, you do you, but you seemt to be struggling with it so maybe you didnt know about it
One question though..
shoot
For https://github.com/home-assistant/core/pull/102211 I added a new file and as for the other PR I tried to add the new file from that PR into .coveragerc.
This time the pre-commit hassfest check spits out some errors I cannot solve.
In this case I would do the change directly on gh to avoid the trouble.
pip3 install numpy==1.26.0
did not help
can you show the full stacktrace?
Validating config_schema... done in 0.78s
Validating dependencies... done in 1.38s
Validating dhcp... done in 0.09s
Validating json... done in 0.00s
Validating manifest... done in 0.02s
Validating mqtt... done in 0.00s
Validating requirements... done in 0.00s
Validating services...Traceback (most recent call last):
File "<frozen runpy>", line 198, in _run_module_as_main
File "<frozen runpy>", line 88, in _run_code
File "/Users/xxx/private/home-assistant-core/script/hassfest/__main__.py", line 245, in <module>
sys.exit(main())
^^^^^^
File "/Users/xxx/private/home-assistant-core/script/hassfest/__main__.py", line 167, in main
plugin.validate(integrations, config)
File "/Users/xxx/private/home-assistant-core/script/hassfest/services.py", line 222, in validate
validate_services(config, integration)
File "/Users/xxx/private/home-assistant-core/script/hassfest/services.py", line 133, in validate_services
services = CORE_INTEGRATION_SERVICES_SCHEMA(data)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 272, in __call__
return self._compiled([], data)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 595, in validate_dict
return base_validate(path, iteritems(data), out)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 387, in validate_mapping
cval = cvalue(key_path, value)
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 229, in _run
return self._exec(self._compiled, value, path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 274, in _exec
return func(path, v)
^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 818, in validate_callable
return schema(data)
^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 272, in __call__
return self._compiled([], data)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 595, in validate_dict
return base_validate(path, iteritems(data), out)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 387, in validate_mapping
cval = cvalue(key_path, value)
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 229, in _run
return self._exec(self._compiled, value, path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 274, in _exec
return func(path, v)
^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 818, in validate_callable
return schema(data)
^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 272, in __call__
return self._compiled([], data)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 595, in validate_dict
return base_validate(path, iteritems(data), out)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 387, in validate_mapping
cval = cvalue(key_path, value)
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 229, in _run
return self._exec(self._compiled, value, path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 353, in _exec
v = func(path, v)
^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 636, in validate_sequence
cval = validate(index_path, value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 818, in validate_callable
return schema(data)
^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 272, in __call__
return self._compiled([], data)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 595, in validate_dict
return base_validate(path, iteritems(data), out)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 387, in validate_mapping
cval = cvalue(key_path, value)
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 636, in validate_sequence
cval = validate(index_path, value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 229, in _run
return self._exec(self._compiled, value, path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/validators.py", line 353, in _exec
v = func(path, v)
^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/venv/lib/python3.11/site-packages/voluptuous/schema_builder.py", line 818, in validate_callable
return schema(data)
^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/homeassistant/helpers/selector.py", line 156, in _validate_supported_features
feature_mask |= _validate_supported_feature(supported_feature)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
what complains about numpy then
File "/Users/xxx/private/home-assistant-core/homeassistant/helpers/selector.py", line 131, in _validate_supported_feature
known_entity_features = _entity_features()
^^^^^^^^^^^^^^^^^^
File "/Users/xxx/private/home-assistant-core/homeassistant/helpers/selector.py", line 91, in _entity_features
from homeassistant.components.camera import CameraEntityFeature
File "/Users/xxx/private/home-assistant-core/homeassistant/components/camera/__init__.py", line 29, in <module>
from homeassistant.components.stream import (
File "/Users/xxx/private/home-assistant-core/homeassistant/components/stream/__init__.py", line 61, in <module>
from .core import (
File "/Users/xxx/private/home-assistant-core/homeassistant/components/stream/core.py", line 14, in <module>
import numpy as np
ModuleNotFoundError: No module named 'numpy'
hassfest-metadata....................................(no files to check)Skipped
hassfest-mypy-config.................................(no files to check)Skipped
@ionic knoll this is what coffeecat meant
pip install -e .
oh right, that's what you said to her too
Problem is unfortunately still the same..
hmm, what happens when you run the script/setup?
runs through without issues
very strange
Then it's complaining about a lot of missing packages
Traceback (most recent call last):
File "<frozen runpy>", line 198, in _run_module_as_main
File "<frozen runpy>", line 88, in _run_code
File "/Users/xxx/private/home-assistant-core/script/hassfest/__main__.py", line 10, in <module>
from . import (
File "/Users/xxx/private/home-assistant-core/script/hassfest/requirements.py", line 14, in <module>
from tqdm import tqdm
ModuleNotFoundError: No module named 'tqdm'
running pip3 install -r requirements.txt
and pip3 install -r requirements_test.txt again, now the manual command works
are you in a venv?
@ionic hollow do you agree with the ViCare typing PR? I updated the rest of the types
I have some doubts about iterables: list[PyViCareDevice]
I think burners, compressors and circuits have no common type
let me finish this commit and I'll take a look
Right I have a small feeling the typing isn't correct from the beginning
At one point we are calling _build_entity with PyViCareDevice as api and at another point we are giving DeviceWithComponent
MAybe this should be two different functions.
I think my conclusion is right
check this out
@property
def unique_id(self) -> str:
"""Return unique ID for this device."""
tmp_id = (
f"{self._device_config.getConfig().serial}-{self.entity_description.key}"
)
if hasattr(self._api, "id"):
return f"{tmp_id}-{self._api.id}"
return tmp_id
DeviceWithComponent has "id", PyViCareDevice doesn't
Do you have the chance to test the integration if it works as expected? Otherwise I can try to run it.
haha, I don't own any ViCare devices
I'm just someone who is looking at certain integrations and be like, oh, we can make this better
Mind if I ask some questions about the context?
Sure
why do you have to put in a heating type when you set up the integration?
At least when we support multiple devices, this wuld make no sense.
Maybe we could fully rely on the automatic detection: https://github.com/somm15/PyViCare/blob/8ba411483a865e074d1146fd1b8b7a8c4f4be122/PyViCare/PyViCareDeviceConfig.py#L70
exactly my point haha
Also, not sure if you noticed, PyViCare has a new version
in a positive way?
v2.28.x also provides the gateways to the heating devices.
Unfortunately, the current serial, which is used as uid is the serial from that gateway and not the heating device.
Right, can you maybe explain a bit more on how the whole system works?
Because when I check the code I see some stuff
gateways, heating devices, circuits
etc
I can describe how it works for me, but Viessmann has multiple, also more modern devices.
So I have two dump heating devices that have an IR interface.
via the interface a gateway box is corrected to my wifi.
as I have two heatings for two flats, I have also two gateways.
two flats?
Yes ๐ two flors
ah right
so this is kind unusual and I guess most users have only one gateway
Or, I think more modern devices have direct wifi access
With my PR, yes
I can't remember now, did your PR also bump the library?
No
So current state gives me my two heating devices
The v2.28 gives me four devices, two heatings and two gateways.
Aaah right
And then the problem is that we can't get the serial from the gateway from the heating anymore?
The gateway also seems to support/expose all features that the heating exposes. So the integration tries to create all the sensors again, which fails due to naming with the same serial

Oh so they have the same serial?
{
"data": [
{
"gatewaySerial": "7571381723160106",
"id": "0",
"boilerSerial": "7452947312545103",
"boilerSerialEditor": "DeviceCommunication",
"bmuSerial": "7509081322740107",
"bmuSerialEditor": "DeviceCommunication",
"createdAt": "2018-06-08T12:48:28.806Z",
"editedAt": "2023-07-04T12:25:31.733Z",
"modelId": "VScotHO1_72",
"status": "Online",
"deviceType": "heating",
"roles": [
"type:boiler",
"type:legacy",
"type:product;VScotHO1"
],
"isBoilerSerialEditable": false
},
{
"gatewaySerial": "7571381723160106",
"id": "gateway",
"boilerSerial": null,
"boilerSerialEditor": null,
"bmuSerial": null,
"bmuSerialEditor": null,
"createdAt": "2021-05-19T06:32:22.925Z",
"editedAt": "2023-10-09T14:23:14.464Z",
"modelId": "Heatbox1",
"status": "Online",
"deviceType": "vitoconnect",
"roles": [
"type:gateway;VitoconnectOpto1",
"type:legacy"
],
"isBoilerSerialEditable": false
}
]
}
wrap them in 3 backticks
This is from the API, first device is the heating device, whuch has a "boilerSerial", that should be used instead
Instead of what
like
is there any benefit in including the gateway as another device?
I would like to see the status of the devices, which of them is offline. and if possible the RSSI
ah so there is a benefit
right
ah, that's also probably for that PR about connectivity
Like if you are up for it, I can help you get this integraton to a nice level
But you're the one that can test it haha
My heating is an old device, so most of the features are not supported. And the library supports a broad bandwidth of heating and other devices.
We can find some more testers, I saw some open issues fly by
I have a nice script from bdraco that you can run in the terminal and it will install a PR as custom component
so we don't need other devs or technical people to test
as long as they know how to execute some command
but more like, I want to know if you are up it
not that I am now just putting you on a task you dont want
So what I am also wondering is, how does the circuits, burners and compressors work?
are those the internals of the heating thing?
Yes, internals.
so they aren't a separate device
No. I also have only one burner and one circuit. Not even domestic hot water, only heating.
And a circuit is the name for the water pipes in your home?
I don't have my own boiler and the one at my parents isn't smart so I don't know the exact term
Yes, and I guess bigger devices can supply multiple circuits
Right right
So now what I am thinking is:
- Fix typing: I think we have to split some stuff up, as for example
apican be eitherPyViCareDeviceorDeviceWithComponent(or subclasses). I think if we straighten this, we can work a bit stricter and decrease the chance we accidentaly create bugs. - Bump the library to 2.28.x: We can create a migration that will change the unique_id to be part of the right device and change the unique_id of the boiler to the boiler_id.
- Auto detect? - This would eliminate the need for the selector in the config flow, enabling us to detect this at startup and connect.
- Add multple device support
Maybe needs more steps, but what do you think?
The config flow is also something that I need to change, the free API has a daily limit, which I reach at about 9 in the evening, therefore I need to adjust the cache lifetime.
yes
Why do you need to adjust the cache lifetime in the config flow tho
It's on 1440 calls a day ๐ฆ
I think you do more calls since you have 2 right
I think so, but I don't know HA so deeply, when it's updating.
Yes!
I was looking at the code and was like
WHAT
since it looks like its doing separate calls for both unit and value for example
I think it's updating every 60 seconds which leads to 1440 calls.
If I set a value I would aleary breach that limit.
I think in your case you can disable polling and poll via an automation
oh but you dont have an coordinator, so that would become a big automation
oh but you dont want that
Like, in that case it would do a call for a lot of stuff
It calls a service for the value of something, and for the unit
I need to bring my kids to bed, but lets continue on this.
yea sure!
I wonder if it would be better to let HA handle the data and updating it.
I mean we have ways to do that
But I am wondering how much calls this lib itself does
My history show an average of 1000calls/day
Sorry, rephrasing, I am not sure how much different calls the lib does
as in self.service.getProperty("heating.dhw.sensors.temperature.hotWaterStorage") vs self.service.getProperty("heating.dhw.sensors.temperature.outlet")
no idea
I think every one is a single call
The thing that does work well in this context is that if your instance doesnt support something, it doesnt create extra polls
I might be wrong about the serial, looks like the serial for the gateway is wrong (https://github.com/somm15/PyViCare/issues/335) so no migration necessary.
That's interesting
Can we finish the typing PR?
I think the only needed change is the HeatingDeviceWithComponent
I think we need to split some stuff up as there is some typing issues in the objects itself
@dataclass
class ViCareSensorEntityDescription(SensorEntityDescription, ViCareRequiredKeysMixin):
"""Describes ViCare sensor entity."""
unit_getter: Callable[[Device], str | None] | None = None
Device can be either a PyViCare or a DeviceWithComponent thing
Do you think there is a change to the library needed or can we solve this via the integration?
So the serial is correct, but in the integration the wrong one is used ๐ฆ PyViCareDeviceConfig.getConfig.serial is used currently and gives the gateway one. The right one would be PyViCareHeatingDevice.getSerial().
I proposed some changes here: https://github.com/home-assistant/core/compare/dev...CFenner:home-assistant-core:CFenner-patch-12
How could a migration script look like?
I think the library could use a little change
for example, they are missing a py.typed file, some kinf of file that shows the lib is typed
and when you do .burners() or whatever, the returnvalue is list[Any]
Hmm, we can just migrate, but in a world where we migrated the entities, is the old serial still used?
Could be used in the future for the gateway itself.