Conversation
304e2a6 to
e3fae0b
Compare
iMicknl
left a comment
There was a problem hiding this comment.
Looks great already! Would be good to think how this could work for LocalServers as well.
e3fae0b to
170393b
Compare
f8ad47e to
835e6bb
Compare
pyoverkiz/const.py
Outdated
| ), | ||
| "somfy_europe_local": lambda gateway_id, token, session: SomfyLocalClient( | ||
| name="Somfy Local(Europe)", | ||
| endpoint=f"https://gateway-{gateway_id}.local:8443/enduser-mobile-web/1/enduserAPI/", |
There was a problem hiding this comment.
This is not flexible enough I would say. There will be users (like me) where bonjour is not working (e.g. they have a VPN / different VNET). It would be good if we can specify a full URL, thus an IP or a .local address is both possible.
There was a problem hiding this comment.
Hum, that’s good to now. So on HA side, you have to enter manually the IP of your box?
56be3a5 to
58959d1
Compare
iMicknl
left a comment
There was a problem hiding this comment.
Had a quick look! Thanks, great improvement.
README.md
Outdated
| print(f"{device.label} ({device.id}) - {device.controllable_name}") | ||
| print(f"{device.widget} - {device.ui_class}") | ||
|
|
||
| await client.register_event_listener() |
There was a problem hiding this comment.
Is this a change compared to our previous behavior? Where we did register this by default during login.
There was a problem hiding this comment.
I forgot to clean this from the previous example. There is still no need to do it.
pyoverkiz/clients/somfy_local.py
Outdated
|
|
||
| async def _login(self, _username: str, _password: str) -> bool: | ||
| """There is no login for Somfy Local""" | ||
| return True |
There was a problem hiding this comment.
Would be good to raise an exception here.
pyoverkiz/overkiz.py
Outdated
| manufacturer="Somfy", | ||
| configuration_url=None, | ||
| session=session, | ||
| username=domain, # not used |
There was a problem hiding this comment.
| username=domain, # not used | |
| username="", # not used |
Shall we just pass an empty string? I am still a bit in doubt if we should not make username and password optional, and add token as an optional string.
There was a problem hiding this comment.
You can check my lastest modifications, username and password are no more within the parent class, but are defined within the specific implementation.
pyoverkiz/overkiz.py
Outdated
|
|
||
| class Overkiz: | ||
| @staticmethod | ||
| def get_client_for( |
There was a problem hiding this comment.
Should we call it get_client_for or create_client. With every call, we create a new client, we don't reuse the existing client in Python like this?
There was a problem hiding this comment.
Yes, that’s a static method which create a new client each time. I will rename it if it confused you.
Co-authored-by: Mick Vleeshouwer <mick@imick.nl>
b4971f3 to
cdafe27
Compare
README.md
Outdated
| print(f"{device.label} ({device.id}) - {device.controllable_name}") | ||
| print(f"{device.widget} - {device.ui_class}") | ||
|
|
||
| await client.register_event_listener() |
There was a problem hiding this comment.
I forgot to clean this from the previous example. There is still no need to do it.
README.md
Outdated
| asyncio.run(main()) | ||
| ``` | ||
|
|
||
| ### Cloud API |
There was a problem hiding this comment.
| ### Cloud API | |
| ### Local API |
pyoverkiz/overkiz.py
Outdated
|
|
||
| class Overkiz: | ||
| @staticmethod | ||
| def get_client_for( |
There was a problem hiding this comment.
Yes, that’s a static method which create a new client each time. I will rename it if it confused you.
pyoverkiz/overkiz.py
Outdated
| manufacturer="Somfy", | ||
| configuration_url=None, | ||
| session=session, | ||
| username=domain, # not used |
There was a problem hiding this comment.
You can check my lastest modifications, username and password are no more within the parent class, but are defined within the specific implementation.
pyoverkiz/overkiz.py
Outdated
|
|
||
| class Overkiz: | ||
| @staticmethod | ||
| def get_client_for( |
| _ssl: bool = field(default=False, init=False) | ||
| token: str = field(repr=lambda _: "***") | ||
|
|
||
| async def _login(self) -> bool: |
There was a problem hiding this comment.
How can we verify if the local login works?
There was a problem hiding this comment.
We can call any command to see if it works.
But I don’t think we should put a check here. There is no login endpoint for the local api, and so we should not call it.
There was a problem hiding this comment.
Let's see how it looks when we have a basic implementation using this branch in core :)
Fix #321
The new main entrypoint is now
Overkiz.get_client_for.OverkizClientdoes not manage anymore the session, it’s up to the calling code to manage it. See my example below.A StrEnum
Serverhas been created to list all the existing supported servers.