Add config flow support to songpal integration by shenxn · Pull Request #34714 · home-assistant/core

shenxn · 2020-04-26T10:38:55Z

Breaking change

Entries for songpal in configuration.yaml have to move out of a platform config to a top level config.

Proposed change

Add config flow support to songpal integration

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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
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

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

The manifest file 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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

probot-home-assistant · 2020-04-26T10:39:03Z

Hey there @rytilahti, mind taking a look at this pull request as its been labeled with a integration (songpal) you are listed as a codeowner for? Thanks!

rytilahti

Thanks a lot for doing this, this is a great improvement! 💯

I tested that the following methods are working:

Existing configuration from yaml gets imported correctly.
If ssdp integration is not loaded, using manual configuration works.
SSDP integration works, but requires a change to ignore bravia televisions.

The following documentation needs to be updated:

Got this error in logs at some point with ssdp autodetected instance:

Apr 26 15:05:18 nuc hass[2714273]: 2020-04-26 15:05:18 ERROR (MainThread) [homeassistant.components.songpal.config_flow] Unable to import songpal configuration (None: None). Please check your endpoint.

rytilahti · 2020-04-26T13:22:47Z

+    def device_info(self):
+        """Return the device info."""
+        return {
+            "identifiers": {(DOMAIN, self.unique_id)},


Should this info define connections, too?

We only need either connections or identifiers.

shenxn · 2020-04-27T00:26:31Z

@rytilahti I couldn't reproduce that error in my dev environment. The import step should never be triggered when there is nothing in the yaml configuration. Can you try to figure out what causes that problem?

rytilahti

Looks good to me, but I'm woefully unaware of how to write proper configuration flows, so let's hope someone else will also take a look at it!

shenxn · 2020-04-27T02:34:30Z

@rytilahti Thanks for the review. I'm gonna make some changes to the name and model part.

rytilahti · 2020-04-27T19:26:03Z

@shenxn do you think it would make sense to add support for unloading also as part of this PR? https://developers.home-assistant.io/docs/config_entries_index/#unloading-entries - this would involve stop listening to the events and cleaning up the hass.data for the domain, so it should be fairly easy to do (and would allow removing the integration without restarts).

Otherwise this looks good to go!

shenxn · 2020-04-27T22:29:55Z

@rytilahti That's a good thing to have. I'll add it today.

shenxn · 2020-04-28T08:10:54Z

@rytilahti Done. I didn't figure out how to easily rediscover the removed device without restarting. But since the device is manually deleted, I think that is not necessary.

raman325 · 2020-05-01T16:19:59Z

-    """Set up the Songpal platform."""
-    if PLATFORM not in hass.data:
-        hass.data[PLATFORM] = {}
+async def async_setup_platform(


To add config flow, the integration can't use platform set up and should be set up as a top level config entry. You can see how I implemented this for vizio here: https://github.com/home-assistant/core/blob/2e6edb0447f6e6c460eb5ab1a1d9c75602eebddd/homeassistant/components/vizio/__init__.py
https://github.com/home-assistant/core/blob/2e6edb0447f6e6c460eb5ab1a1d9c75602eebddd/homeassistant/components/vizio/media_player.py

This function can be removed.

raman325 · 2020-05-01T16:42:48Z

Just to note, by changing how the configuration is defined, this will be a breaking change. But every config flow implementation from a former platform config integration is a breaking change, so no concerns there 😄

This reverts commit 9be076a.

raman325

LGTM

raman325 · 2020-05-04T13:39:59Z

@shenxn please rebase on dev and force push to resolve the conflict

shenxn · 2020-05-05T08:58:31Z

@raman325 Done. Should be good to merge.

rytilahti · 2020-05-06T22:52:22Z

Thank you @shenxn for the PR and for your willingness to become a codeowner, the more eyeballs, the better. Thank you @raman325 for your reviews, I think it's time to get this merged now! 🎉

MartinHjelmare

Some comments for the future.

MartinHjelmare · 2020-05-06T23:01:33Z

+    endpoint = config_entry.data[CONF_ENDPOINT]

-    if endpoint in hass.data[PLATFORM]:
+    if endpoint in hass.data[DOMAIN]:


This is already checked in the config flow.

MartinHjelmare · 2020-05-06T23:03:56Z

+"""Test the songpal config flow."""
+import copy
+
+from asynctest import MagicMock, patch


Please import all mocks and patch from tests.async_mock.

MartinHjelmare · 2020-05-06T23:08:02Z

        }

-        for device in hass.data[PLATFORM].values():
+        for device in hass.data[DOMAIN].values():


We can use the platform service registration helper and avoid storing the entities.

https://developers.home-assistant.io/docs/dev_101_services#entity-services

homeassistant added the cla-signed label Apr 26, 2020

probot-home-assistant Bot added has-tests integration: songpal type: new-feature labels Apr 26, 2020

probot-home-assistant Bot assigned rytilahti Apr 26, 2020

This was referenced Apr 26, 2020

Add and fix sony songpal home-assistant/brands#1515

Merged

Handles InvalidURL and ClientConnectionError exceptions rytilahti/python-songpal#72

Merged

rytilahti requested changes Apr 26, 2020

View reviewed changes

shenxn mentioned this pull request Apr 27, 2020

Add Config Flow and SSDP to songpal home-assistant/home-assistant.io#13183

Merged

8 tasks

shenxn marked this pull request as ready for review April 27, 2020 01:01

shenxn requested a review from rytilahti April 27, 2020 01:36

rytilahti approved these changes Apr 27, 2020

View reviewed changes

Comment thread homeassistant/components/songpal/config_flow.py Outdated

Comment thread homeassistant/components/songpal/config_flow.py Outdated

Comment thread homeassistant/components/songpal/config_flow.py

Comment thread homeassistant/components/songpal/media_player.py Outdated

rytilahti added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Apr 27, 2020

shenxn requested a review from rytilahti April 27, 2020 04:46

shenxn force-pushed the songpal-configflow branch from 7294dcd to f80a797 Compare April 28, 2020 07:06

raman325 reviewed Apr 28, 2020

View reviewed changes

Comment thread homeassistant/components/songpal/config_flow.py Outdated

raman325 reviewed Apr 28, 2020

View reviewed changes

Comment thread homeassistant/components/songpal/strings.json Outdated

raman325 reviewed Apr 28, 2020

View reviewed changes

Comment thread .coveragerc Outdated

shenxn requested a review from raman325 April 29, 2020 01:56

raman325 requested changes May 1, 2020

View reviewed changes

Comment thread homeassistant/components/songpal/config_flow.py Outdated

Comment thread homeassistant/components/songpal/config_flow.py Outdated

raman325 requested changes May 1, 2020

View reviewed changes

rytilahti approved these changes May 1, 2020

View reviewed changes

raman325 added the breaking-change label May 1, 2020

shenxn and others added 17 commits May 4, 2020 06:30

Remove songpal config flow from .coveragerc omit

0ba6220

Bump python-songpal to 0.12 and fix exception handling

042e9b6

Revert "Improve songpal/set_sound_setting service"

6a5f0e2

This reverts commit 9be076a.

Code style fix

9ab1d57

Add connections to device_info

d67b5d7

Fix pylint

d4b173d

Ignore braava tv

3fcc46e

Fix test warning

a22a2ab

Add @shenxn as codeowner

b821994

Remove model from configuration data

3199a56

Get name from device in user step

a79e36a

Add unload entry support

ac386fa

Delete translations as it will get generated as part of CI

0732fda

Code cleanup

c3b6f62

Fix typo

6c88b9b

Remove _show_setup_form

410f960

Change configuration from media_player to songpal

bf01b2b

raman325 approved these changes May 4, 2020

View reviewed changes

shenxn force-pushed the songpal-configflow branch from 086d7ee to bf01b2b Compare May 4, 2020 13:44

balloob added new-feature and removed type: new-feature labels May 4, 2020

raman325 approved these changes May 5, 2020

View reviewed changes

rytilahti merged commit 33077f0 into home-assistant:dev May 6, 2020

MartinHjelmare reviewed May 6, 2020

View reviewed changes

shenxn mentioned this pull request May 7, 2020

Songpal code and test improvement #35318

Merged

20 tasks

shenxn deleted the songpal-configflow branch May 7, 2020 05:55

lock Bot locked and limited conversation to collaborators May 21, 2020

Uh oh!

Conversation

shenxn commented Apr 26, 2020 • edited by raman325 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Apr 26, 2020

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rytilahti Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shenxn commented Apr 27, 2020

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shenxn commented Apr 27, 2020

Uh oh!

rytilahti commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shenxn commented Apr 27, 2020

Uh oh!

shenxn commented Apr 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman325 May 1, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raman325 commented May 1, 2020

Uh oh!

raman325 left a comment

Choose a reason for hiding this comment

Uh oh!

raman325 commented May 4, 2020

Uh oh!

shenxn commented May 5, 2020

Uh oh!

rytilahti commented May 6, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare May 6, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare May 6, 2020

Choose a reason for hiding this comment

shenxn commented Apr 26, 2020 •

edited by raman325

Loading

Example entry for `configuration.yaml`:

rytilahti commented Apr 27, 2020 •

edited

Loading