Abstract device model exteded by model name (identifier)#64
Abstract device model exteded by model name (identifier)#64rytilahti merged 2 commits intorytilahti:masterfrom
Conversation
mirobo/device.py
Outdated
| import socket | ||
| import logging | ||
| from typing import Any, List | ||
| from typing import Any, List, Optional |
There was a problem hiding this comment.
It's used as annotation. This is fine.
There was a problem hiding this comment.
Indeed, I added a noqa for those cases for now.
mirobo/device.py
Outdated
|
|
||
| def model(self): | ||
| return self.info().model | ||
|
|
There was a problem hiding this comment.
Although I think it's fine to expose the model information directly in DeviceInfo, I'm not sure if the Device interface should be extended to expose this too. What is the usecase you have in your mind and would it be too bad to access the model directly there on the call-site?
There was a problem hiding this comment.
I want to provide the model of the device as sensor attribute at home assistant. It's good to know as long there is no discovery process and the user has to decide which component to use. Especially if various generations of a device exists I would like to handle the firmware differences properly.
There was a problem hiding this comment.
Makes sense, but would it be too bad to fetch info only once and use the DeviceInfo to get extract the information out from it, making the fetch explicit and avoid doing multiple I/O calls when accessing other information from the object.
mirobo/device.py
Outdated
|
|
||
| @property | ||
| def netif(self): | ||
| def netif(self) -> str: |
There was a problem hiding this comment.
IIRC these are not strings but rather dicts, maybe it's a good idea to add an example output into the class' docstring.
There was a problem hiding this comment.
My fault. I wil add a proper docstring for the future.
There was a problem hiding this comment.
Not a problem. Looks like I lost my connectivity to my raspberry pi running as a controller, so I can't do much testing on this for now. If you can, could you please add the output of the info() to the doctstring of DeviceInfo class (that will also help when creating unit tests at some point).
syssi
force-pushed
the
feature/retrieve-device-model
branch
from
0faacbe to
fd715bd
Compare
|
Mmh, looks like coveralls got stuck for some reason. Anyway, this all looks fine so I'm merging it. |
4 participants
No description provided.