Add base_device_url and corresponding tests on Device model

[nyroDev]

After starting the implementation on HA component, I figured out that we where missing the base_device_url.
We could recreate it there, but I think it's better to have always available on the device info.

[nyroDev]

@iMicknl
Once this PR is merged and published, I'll be able to add new PR on HA.
Everything is ready here, I'll just have to update the version number

[iMicknl]

Let's wait for a 2nd approver, and then we will merge + release.

[tetienne]

I don't see the pro of such property. It will be confusing too have a device url and a base url. From the name, the diff is not obvious.

[nyroDev]

I don't see the pro of such property. It will be confusing too have a device url and a base url. From the name, the diff is not obvious.

@tetienne This is the name currently used in HomeAssistant (as well as in HA-Tahoma)

[nyroDev]

I don't see the pro of such property. It will be confusing too have a device url and a base url. From the name, the diff is not obvious.

@tetienne Regarding why we need it, please see the linked PR for Home Assistant

[nyroDev]

Hi @tetienne did you have time to read my replies?
Any suggestion for the name maybe?

[tetienne]

I have a suggestion related to the various addition we did on the Device model.
As we have IMO too many properties directly within Device, what do you think if we create a DeviceUrl class:

class DeviceUrl:

  def __init__(device_url: str):
    self.device_url = device_url
    self.base = ...
    self.index = ...

It will move the boilerplate, and ease the logic. Of course, we should declare it as a breaking change.

In the client code:

device.url.full
device.url.base
device.url.index

[nyroDev]

I have a suggestion related to the various addition we did on the Device model. As we have IMO too many properties directly within Device, what do you think if we create a DeviceUrl class:

class DeviceUrl:

  def __init__(device_url: str):
    self.device_url = device_url
    self.base = ...
    self.index = ...

It will move the boilerplate, and ease the logic. Of course, we should declare it as a breaking change.

In the client code:

device.url.full
device.url.base
device.url.index

@tetienne makes sense to me, but will it be worth all the rewriting?

Anyway, if accepted, we should have on this DeviceUrl :

• full
• base (same as full without #index)
• protocol
• gateway
• address
• index (int)
• is_sub (bool)

@iMicknl what do you think?

[nyroDev]

@iMicknl @tetienne What should I do here?

[tetienne]

IMO there is too many properties now within Device and we don’t match anymore what’s returned by Overkiz.

While previously I suggested to change the type of device_url, it will introduce a breaking change. To avoid this, we can create a DeviceUrlParser class:

class DeviceUrlParser:
  def __init__(device_url: str):
    self.device_url = device_url
    self.base = ...
    self.index = ...
    self.protocol = ...
    self.gateway = ...
    ...

So in the client code, you retrieve the device_url, and then instantiate a DeviceUrlParser. We keep the model close from Overkiz, and we introduce an utility class.
In the changelog, we can flag as deprecated the replaced properties within Device.

[nyroDev]

IMO there is too many properties now within Device and we don’t match anymore what’s returned by Overkiz.

While previously I suggested to change the type of device_url, it will introduce a breaking change. To avoid this, we can create a DeviceUrlParser class:

class DeviceUrlParser:
  def __init__(device_url: str):
    self.device_url = device_url
    self.base = ...
    self.index = ...
    self.protocol = ...
    self.gateway = ...
    ...

So in the client code, you retrieve the device_url, and then instantiate a DeviceUrlParser. We keep the model close from Overkiz, and we introduce an utility class. In the changelog, we can flag as deprecated the replaced properties within Device.

It will still be a breaking change, as you're removing properties that was there before.

[tetienne]

We can do this in two release. First the new class then the remove.

[nyroDev]

@iMicknl What do you think about this PR and the related on HA ready

[iMicknl]

@nyroDev sorry for the long due merge/review. Can you perhaps rebase on main and see if CI/CD works again? I will have a look at this PR again. If it doesn't have any breaking changes, I am fine with it.

[nyroDev]

@nyroDev sorry for the long due merge/review. Can you perhaps rebase on main and see if CI/CD works again? I will have a look at this PR again. If it doesn't have any breaking changes, I am fine with it.

Done.