Add config flow to frontier_silicon #64365

wlcrs · 2022-01-18T12:59:53Z

Breaking change

Frontier Silicon integration moves to configuration via Config Flow. Users need to remove their existing integration from configuration.yaml .

Proposed change

Update to FSAPI 0.2.0, which is a large overhaul of the previous implementation. (cfr: wlcrs/python-afsapi@0.0.4...0.2.0 )

It does not longer interfere with the use of the UNDOK application to control the radio.

This PR is the first in a series that will add new functionality to this integration (oa. selecting presets). As per the instructions I'm splitting these out in multiple PR's to ease the process.

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

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 (cfr. Update frontier_silicon documentation home-assistant.io#21238 )

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

The integration reached or maintains the following Integration Quality Scale:

[] No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

.coveragerc

iMicknl

Thanks! I had a quick look to get you started. Please add untested files to .coveragerc or add tests. Currently this PR is hard to review since there are warnings due to untested code.

homeassistant/components/frontier_silicon/__init__.py

homeassistant/components/frontier_silicon/media_player.py

homeassistant/components/frontier_silicon/manifest.json

homeassistant/components/frontier_silicon/config_flow.py

TopdRob · 2022-02-09T23:48:55Z

Tested this with a Philips TAPR802/12 radio and it works without any problems.

homeassistant/components/frontier_silicon/__init__.py

wlcrs · 2023-03-09T08:18:00Z

Hi @epenet , thank you for your feedback on this PR in the last few days. Can you please finalize your review so that we have ample time to add the other config flows in 2023.4 release? I've already used your feedback from this PR to improve the code in the other flows (+their tests), so it should be smooth sailing.

homeassistant/components/frontier_silicon/media_player.py

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

epenet

LGTM, but I'd like a second pair of eyes

Kane610 · 2023-03-09T16:52:41Z

homeassistant/components/frontier_silicon/media_player.py

+
+    YAML is deprecated, and imported automatically.
+    SSDP discovery is temporarily retained - to be refactor subsequently.
+    """
    if discovery_info is not None:


You should only do the import in the platform setup and add the media player entities only from the setup_entry method. Right now you add the entities twice.

In which case are the entities loaded twice? I fail to see what problem you are mentioning?

In case of an SSDP discovery: they are added here in async_setup_platform and not in setup_entry.
In case of an import of the old YAML: the entity is migrated here but not added. It is subsequently added from setup_entry.
In case of an entity added via the config flow: only added via setup_entry.

In which case are the entities loaded twice? I fail to see what problem you are mentioning?

In case of an SSDP discovery: they are added here in async_setup_platform and not in setup_entry. In case of an import of the old YAML: the entity is migrated here but not added. It is subsequently added from setup_entry. In case of an entity added via the config flow: only added via setup_entry.

Aah sorry, I apparently forgot the nomenclature for the old way of loading entities. Still good I mentioned it as you should also move the SSDP discovery from the old discovery way to the config flow, there shouldn't be any in-between steps, its either old platform or new way. Can't say that I have any other comments apart from this though.

Ah - that was a recommendation that I made, to reduce the size of the PR.
I felt it was easier to review without SSDP, and then migrate SSDP as a follow-up PR.

Ok. And this was the future config entry PR mentioned? If that is the case I can be ok with that

Yes, I'll be creating a PR for the following branch from the moment this one is merged: wlcrs/home-assistant-core@fsapi-0.2.0-update...wlcrs:home-assistant-core:frontier_silicon_ssdp

epenet · 2023-03-10T09:26:39Z

Thanks @wlcrs 👍
And thanks @Kane610 for the second opinion👍

frenck

Please address these comments in a new PR.

Thanks 👍

../Frenck

frenck · 2023-03-10T12:04:43Z

homeassistant/components/frontier_silicon/config_flow.py

+    def __init__(self) -> None:
+        """Initialize flow."""
+
+        self._webfsapi_url: str | None = None
+        self._name: str | None = None
+        self._unique_id: str | None = None


These can be set up as class variables instead.

frenck · 2023-03-10T12:05:28Z

homeassistant/components/frontier_silicon/config_flow.py

+        try:
+            afsapi = AFSAPI(self._webfsapi_url, import_info[CONF_PIN])
+
+            self._unique_id = await afsapi.get_radio_id()


Why is this assigned to a class variable? It isn't used anywhere later on?

frenck · 2023-03-10T12:07:35Z

homeassistant/components/frontier_silicon/config_flow.py

+        assert self._name is not None
+        assert self._webfsapi_url is not None


These asserts are not needed. It means the typing isn't correct. Please correct that instead and remove these asserts.

frenck · 2023-03-10T12:08:25Z

homeassistant/components/frontier_silicon/config_flow.py

+            errors=errors,
+        )
+
+    async def _create_entry(self, pin: str | None = None) -> FlowResult:


IMHO this method adds complexity that is not needed.
Instead of a simple async_create_entry call, we not have additional variables and calls over the place (that are out of standard).

I suggest to just remove this method fully, as the only thing it does is making a single call.

frenck · 2023-03-10T12:09:30Z

homeassistant/components/frontier_silicon/strings.json

+    "flow_title": "{name}",
+    "step": {
+      "user": {
+        "title": "Frontier Silicon Setup",


This is already implied, please remove title.

frenck · 2023-03-10T12:09:44Z

homeassistant/components/frontier_silicon/strings.json

@@ -0,0 +1,35 @@
+{
+  "config": {
+    "flow_title": "{name}",


This is the default flow title and should thus be removed.

frenck · 2023-03-10T12:11:35Z

tests/components/frontier_silicon/test_config_flow.py

+
+    assert result3["type"] == FlowResultType.FORM
+    assert result2["step_id"] == "device_config"
+    assert result3["errors"] == {"base": result_error}


This test is incomplete. Please finish the test with the user doing an second attempt and finishes the config flow until the create entry.

This ensures there are no side-effects from the error.

frenck · 2023-03-10T12:11:56Z

tests/components/frontier_silicon/test_config_flow.py

+
+    assert result2["type"] == FlowResultType.FORM
+    assert result2["step_id"] == "user"
+    assert result2["errors"] == {"base": result_error}


This test is incomplete. Please finish the test with the user doing an second attempt and finishes the config flow until the create entry.

This ensures there are no side-effects from the error.

bdraco · 2023-04-26T17:32:19Z

homeassistant/components/frontier_silicon/__init__.py

+    try:
+        await afsapi.get_power()
+    except FSConnectionError as exception:
+        raise PlatformNotReady from exception


This needs to be ConfigEntryNotReady for it to work as intended

MartinHjelmare · 2023-04-27T08:13:35Z

homeassistant/components/frontier_silicon/strings.json

+      "unknown": "[%key:common::config_flow::error::unknown%]"
+    },
+    "abort": {
+      "already_configured": "[%key:common::config_flow::abort::already_configured_device%]"


We're missing abort reason strings for cannot_connect, invalid_auth and unknown.

homeassistant added the cla-signed label Jan 18, 2022

probot-home-assistant bot added breaking-change code-quality core dependency has-tests integration: discovery integration: frontier_silicon labels Jan 18, 2022

MartinHjelmare changed the title ~~Update frontier_silicon~~ Add config flow to frontier_silicon Jan 18, 2022

wlcrs mentioned this pull request Jan 18, 2022

Update frontier_silicon documentation home-assistant/home-assistant.io#21238

Merged

8 tasks

iMicknl reviewed Jan 19, 2022

View reviewed changes

.coveragerc Show resolved Hide resolved

iMicknl reviewed Jan 19, 2022

View reviewed changes

wlcrs marked this pull request as draft January 19, 2022 19:23

iMicknl reviewed Jan 21, 2022

View reviewed changes

homeassistant/components/frontier_silicon/config_flow.py Outdated Show resolved Hide resolved

iMicknl reviewed Jan 21, 2022

View reviewed changes

homeassistant/components/frontier_silicon/config_flow.py Outdated Show resolved Hide resolved

iMicknl reviewed Jan 21, 2022

View reviewed changes

homeassistant/components/frontier_silicon/config_flow.py Outdated Show resolved Hide resolved

wlcrs marked this pull request as ready for review January 21, 2022 21:13

wlcrs mentioned this pull request Jan 22, 2022

Allow selection of preset channels zhelev/python-afsapi#4

Closed

wlcrs force-pushed the fsapi-0.2.0-update branch from 0fb58be to 659c348 Compare January 27, 2022 15:38

wlcrs marked this pull request as draft January 27, 2022 16:59

wlcrs force-pushed the fsapi-0.2.0-update branch from 659c348 to 81f0a7c Compare January 27, 2022 18:48

wlcrs marked this pull request as ready for review January 27, 2022 20:00

wlcrs mentioned this pull request Feb 10, 2022

project-bot does not always react to homeassistant bot / probot-home-assistant actions #66260

Closed

buzz-tee mentioned this pull request Feb 20, 2022

Switch channels via media browser and play_media service #58960

Closed

22 tasks

frenck added the config-flow This integration migrates to the UI by adding a config flow label Mar 19, 2022

wlcrs requested a review from a team as a code owner April 5, 2022 15:24

iMicknl reviewed Apr 5, 2022

View reviewed changes

homeassistant/components/frontier_silicon/__init__.py Outdated Show resolved Hide resolved

wlcrs force-pushed the fsapi-0.2.0-update branch from de7e8cb to a49abee Compare April 5, 2022 15:39

wlcrs mentioned this pull request Apr 5, 2022

Upgrade frontier_silicon library to AFSAPI 0.2.4 #69371

Merged

21 tasks

epenet requested changes Mar 9, 2023

View reviewed changes

homeassistant/components/frontier_silicon/media_player.py Outdated Show resolved Hide resolved

home-assistant bot marked this pull request as draft March 9, 2023 08:27

Update homeassistant/components/frontier_silicon/media_player.py

08896fd

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

epenet approved these changes Mar 9, 2023

View reviewed changes

epenet added second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. deprecation Indicates a breaking change to happen in the future and removed breaking-change integration: discovery dependency code-quality core labels Mar 9, 2023

Kane610 requested changes Mar 9, 2023

View reviewed changes

wlcrs marked this pull request as ready for review March 9, 2023 19:22

home-assistant bot requested review from epenet and Kane610 March 9, 2023 19:22

Kane610 approved these changes Mar 9, 2023

View reviewed changes

epenet merged commit b8bda93 into home-assistant:dev Mar 10, 2023

wlcrs mentioned this pull request Mar 10, 2023

Migrate ssdp to config_flow for frontier_silicon #89496

Merged

20 tasks

frenck reviewed Mar 10, 2023

View reviewed changes

twalcari mentioned this pull request Mar 10, 2023

Address late review comments for frontier_silicon config flow #89507

Merged

20 tasks

wlcrs deleted the fsapi-0.2.0-update branch March 10, 2023 13:24

github-actions bot locked and limited conversation to collaborators Mar 11, 2023

bdraco reviewed Apr 26, 2023

View reviewed changes

MartinHjelmare reviewed Apr 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config flow to frontier_silicon #64365

Add config flow to frontier_silicon #64365

wlcrs commented Jan 18, 2022 •

edited

Loading

iMicknl left a comment

TopdRob commented Feb 9, 2022

wlcrs commented Mar 9, 2023

epenet left a comment

Kane610 Mar 9, 2023

wlcrs Mar 9, 2023

Kane610 Mar 9, 2023 •

edited

Loading

epenet Mar 9, 2023

Kane610 Mar 9, 2023

wlcrs Mar 9, 2023

epenet commented Mar 10, 2023

frenck left a comment

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

frenck Mar 10, 2023

bdraco Apr 26, 2023

MartinHjelmare Apr 27, 2023

		assert self._name is not None
		assert self._webfsapi_url is not None

Add config flow to frontier_silicon #64365

Add config flow to frontier_silicon #64365

Conversation

wlcrs commented Jan 18, 2022 • edited Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

iMicknl left a comment

Choose a reason for hiding this comment

TopdRob commented Feb 9, 2022

wlcrs commented Mar 9, 2023

epenet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kane610 Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epenet commented Mar 10, 2023

frenck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlcrs commented Jan 18, 2022 •

edited

Loading

Kane610 Mar 9, 2023 •

edited

Loading