core icon indicating copy to clipboard operation
core copied to clipboard
Published 1 week ago • verified publisher icon home-assistant

Home connect overhaul

Open Diegorro98 opened this issue 1 year ago • 10 comments

Breaking change

Unique entity IDs are no generated based on the BSH settings/status key.

Proposed change

  • Program switch entities are now converged into a program selection entity
  • Unique IDs are now based on BSH setting/status key
  • Translation keys based on BSH setting/status key
  • Added icons
  • Added entity descriptions to all platforms
  • Entities are added only if the status or the setting key exist on the appliance, except for select program, which will be created if there are programs available for the device. (Before, for example, Childlock always was available, although the appliance was not capable)
  • Deleted device models from api.py becase it was required to be updated when new types of appliances were available at Home Connect API
    • As a result of this change and the entity descriptions, Cleaning Robot appliances between others (and future appliance types), are now supported
  • Added Number platform for entities that allows, for example, adjust the temperature of the appliance
  • Added Time platform for appliances that support alarm clock
  • Added information to some action descriptions about where the setting keys, status keys, and the possible values can be found (https://api-docs.home-connect.com/)
  • Light entities can support RGB and use its attributes

Maybe there are too many changes for one PR, if this is an problem, I will try the divide the big commit into multiple commits and make the necessary PRs.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [X] Breaking change (fix/feature causing existing functionality to break)
  • [X] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue:
    • #103368
    • #91216
  • This PR might conflict with this PR: #125490

Checklist

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the development checklist
  • [ ] I have followed the perfect PR recommendations
  • [X] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [X] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [X] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [X] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Diegorro98 avatar Sep 13 '24 12:09 Diegorro98

Hey there @davidmstraub, mind taking a look at this pull request as it has been labeled with an integration (home_connect) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of home_connect can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign home_connect Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Sep 13 '24 12:09 home-assistant[bot]

Okay I love this change, but we need to split it up into smaller pieces and PRs in order to get this in the best way

joostlek avatar Sep 13 '24 12:09 joostlek

@joostlek okey, let's do it!

How about first a PR where I do the following changes from the above list:

  • Translation keys based on BSH setting/status key
  • Added icons
  • Added entity descriptions to all platforms
  • Entities are added only if the status or the setting key exist on the appliance, except for select program, which will be created if there are programs available for the device.
  • Deleted device models from api.py becase it was required to be updated when new types of appliances were available at Home Connect API

Because these changes are directly related, the other ones can go in later PRs.

Also, is it ok if I create PRs with commits from other of my PRs (and leave them as a draft), and when the PRs that contains commits are merged, rebase them?

Diegorro98 avatar Sep 13 '24 12:09 Diegorro98

I think it's good that we explain the rules upfront:

  • Changing one entity to another type (Program switch entities are now converged into a program selection entity) should happen with a deprecation of 6 months. So we can't remove them and add them in the same PR
  • Unique IDs are now based on BSH setting/status key, unless the API changed, we need to migrate unique ids, we can't just change them and have everyone update their automations.

So I would suggest that we first start with improving code quality, maybe look at improving the tests (maybe add snapshot tests, which are awesome by the way).

joostlek avatar Sep 13 '24 12:09 joostlek

Okey, I think I understand. I tried to divide this PR and I got 6 different PRs:

  1. Use BSH keys as unique IDs and translation keys, will include:
    • Unique ID migration
    • Strings for the entities that were already there
  2. (Is easier if depends on 1) Entity description improvements:
    • Add entity descriptions for the current entities and other entities that are not yet
    • Device models will be deleted
    • Entities are added only if the status or the setting/status key exist on the appliance.
  3. Deprecation of program selection switches and add program select entity
  4. Number and Time platforms
  5. Light improvements
  6. Add information to action descriptions

Diegorro98 avatar Sep 13 '24 13:09 Diegorro98

I think that might work. I can't guarantee it though as I am not familiar with home_connect other than the PR I reviewed last week

joostlek avatar Sep 13 '24 13:09 joostlek

These changes are great, I've been running a branch from my local repo trying to make similar changes and introducing entities as they come up. Looking forward to seeing the PRs come in 👍

beastie29a avatar Sep 13 '24 21:09 beastie29a

@beastie29a I see you now opened a new PR, I am not sure how many more you will add, but once these PRs come up we probably need to coordinate them to make sure we don't have to rebase every time

joostlek avatar Sep 17 '24 09:09 joostlek

@beastie29a I see you now opened a new PR, I am not sure how many more you will add, but once these PRs come up we probably need to coordinate them to make sure we don't have to rebase every time

@joostlek, sounds good. I will come up with a plan for some more work I was accomplishing to get the timing correct with the releases. Thanks for the heads up 👍🏼

beastie29a avatar Sep 18 '24 00:09 beastie29a

Also, it would be nice if we could raise the code quality first, like adding more types to functions and make it nice :)

joostlek avatar Sep 18 '24 09:09 joostlek