Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add config flow to frontier_silicon #64365
Add config flow to frontier_silicon #64365
Changes from 36 commits
427a711
1436787
7fc104b
2c3456a
f7bcbff
65be2b6
1bae242
482f0e2
44c1a28
1fa59e2
5170de2
4a17ac8
d4ca822
e527195
ddf694e
f269111
577a6f3
808217b
447edf9
12ef02b
956d8b0
62caf1e
842f7e2
1b8ef30
eb5abb8
e087f21
43c715e
da0a7e1
e215fdd
1e69338
6fcb5c5
c956220
e174b53
382ed4f
df07d73
06090ad
08896fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be
ConfigEntryNotReady
for it to work as intendedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be set up as class variables instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this assigned to a class variable? It isn't used anywhere later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These asserts are not needed. It means the typing isn't correct. Please correct that instead and remove these asserts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 insetup_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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. And this was the future config entry PR mentioned? If that is the case I can be ok with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default flow title and should thus be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already implied, please remove
title
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing abort reason strings for
cannot_connect
,invalid_auth
andunknown
.