Conversation
|
Hey there @cereal2nd, @brefra, mind taking a look at this pull request as it has been labeled with an integration ( |
There was a problem hiding this comment.
Not sure about this one. This integration is not strictly typed, we don't have to change this right now. I rather see it being fixed than moved over into newer structures while ignoring typing.
There was a problem hiding this comment.
all parts in this tuple are strings, so the only problem is that we have a 3 part tuple instead of a 2 part?
if thats the case we can safely remove the ' self._channel.get_module_serial()' part.
There was a problem hiding this comment.
I've updated the identifiers.
There was a problem hiding this comment.
How can we safely remove that? This changes the resulting identifier? Doesn't this require to migrated?
There was a problem hiding this comment.
Has a migration of this kind been implemented in another component that I can get inspiration from?
There was a problem hiding this comment.
If not, then we keep the original identifiers and remove the type hint on the method -> DeviceInfo
There was a problem hiding this comment.
This looks like a material change in the identifier ?
bdraco
left a comment
There was a problem hiding this comment.
This one needs to have its identifier migrated since there is a material change here.
Agreed, but is there an example out there? |
|
|
Here is the migration code, to migrate from the old to the new format |
Thanks @cereal2nd, I've added it to the initialisation routine. |
|
@bdraco can you have a look at the latest changes? |
bdraco
left a comment
There was a problem hiding this comment.
The code looks like it will work, but we should add a test since migrations are always high risk operations.
|
@epenet can you look in writing a testcase? i have no idea where to start |
62abba1 to
58f055a
Compare
|
@cereal2nd / @bdraco I have added the tests for migration in |
bdraco
left a comment
There was a problem hiding this comment.
Looks good. One small comment above 👍
bdraco
left a comment
There was a problem hiding this comment.
Should be good to go if CI passes 👍
collecting ...
tests/components/velbus/test_config_flow.py ✓✓✓✓ 67% ██████▋
tests/components/velbus/test_init.py ✓✓ 100% ██████████
Results (0.28s):
6 passed
|
CI passed :) |
|
Thanks @bdraco |
Proposed change
Use DeviceInfo in
velbus.Split out from #58445 to give opportunity for code owners to review.
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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: