Skip to content

Allow device type WasherDryer for home_connect component#89113

Closed
DennisGaida wants to merge 4 commits into
home-assistant:devfrom
DennisGaida:patch-1
Closed

Allow device type WasherDryer for home_connect component#89113
DennisGaida wants to merge 4 commits into
home-assistant:devfrom
DennisGaida:patch-1

Conversation

@DennisGaida
Copy link
Copy Markdown

@DennisGaida DennisGaida commented Mar 3, 2023

Proposed change

This PR adds the device type "WasherDryer" or a washing machine that can also dry. According to documentation it can pretty much do everything a dryer as well as what a washer can do. Additionally there are a couple of WasherDryer programs.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

All this PR does is add the possibility to connect a WasherDryer type device. What is currently lacking is one thing:

Currently the HA switch for programs is just generated by splitting the program name (e.g. LaundryCare.Washer.Program.Cotton) at the last dot. Unfortunately this creates duplicate names such that LaundryCare.Dryer.Program.Cotton and LaundryCare.WasherDryer.Program.Cotton would all just be "Cotton"

This means if all programs were added to the current implementation you would get errors such as these:

2023-03-03 16:08:52.736 ERROR (MainThread) [homeassistant.components.switch] Platform home_connect does not generate unique IDs. ID BOSCH-<uniqueid>-Program Cotton already exists - ignoring switch.washer_dryer_program_cotton
2023-03-03 16:08:52.740 ERROR (MainThread) [homeassistant.components.switch] Platform home_connect does not generate unique IDs. ID BOSCH-<uniqueid>-Program Mix already exists - ignoring switch.washer_dryer_program_mix

I have currently commented out the duplicate keys, which means there is commented out code in this PR. To really fix this the whole home_connect component would need to be rewritten to be able to distinguish between the different program types (i.e. instead of "Program Cotton" the switch would need to be named "Program Cotton Dryer", "Program Cotton Washer" and "Program Cotton WasherDryer" or something like that) - the will be a complete rework of the home_connect component.

The actual best practice is to use the programs/get_available_programs endpoint per device instead of hardcoding the programs like the current implementation of the component does - again a complete rework of the home_connect component.

I believe this PR would be a good start to make the WasherDryer device type just work™, but ultimately the component should be rewritten to get the available programs via the API and create switches based on that and name these switches sensible instead of just splitting the key (the programs actually all have friendly names when acquired via the API - still not unique, but friendly).

Checklist

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

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

  • Documentation added/updated for [www.home-assistant.io][docs-repository]

The documentation currently does not specify supported devices.

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

  • The [manifest file][manifest-docs] has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • 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.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Mar 3, 2023

Hi @DennisGaida

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Mar 3, 2023

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.

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 6, 2023

I've marked this PR, as some jobs in our CI are failing. Please check the output of those jobs to see what went wrong and adjust the PR accordingly.

Please un-draft it once the CI passes and it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

@frenck frenck marked this pull request as draft March 6, 2023 10:59
@DennisGaida DennisGaida marked this pull request as ready for review March 6, 2023 15:48
@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Mar 14, 2023
{"name": "LaundryCare.Washer.Program.SportFitness"},
{"name": "LaundryCare.Washer.Program.Towels"},
{"name": "LaundryCare.Washer.Program.WaterProof"},
# {"name": "LaundryCare.Dryer.Program.Cotton"},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there commented out code here?
Either remove it if it's not needed, or uncomment it if it's needed.

@home-assistant
Copy link
Copy Markdown
Contributor

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@stickpin
Copy link
Copy Markdown
Contributor

This PR is outdated, due to the latest changes done in the Home Connect integration programs fetching mechanism.
Please see: #90673

@DennisGaida
Copy link
Copy Markdown
Author

Awesome, I didn't see any update concerning the statically typed list of program names which I didn't like at all. So I fully support the other / newer PR. Hopefully that one will be merged soon then...

@github-actions github-actions Bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed integration: home_connect new-feature Quality Scale: No score smash Indicator this PR is close to finish for merging or closing

Projects

None yet

4 participants