Skip to content

Refactor Freebox : add config flow + temperature sensor + signal dispatch#11712

Merged
frenck merged 3 commits into
home-assistant:nextfrom
Quentame:freebox/config-flow
Mar 12, 2020
Merged

Refactor Freebox : add config flow + temperature sensor + signal dispatch#11712
frenck merged 3 commits into
home-assistant:nextfrom
Quentame:freebox/config-flow

Conversation

@Quentame
Copy link
Copy Markdown
Member

Description:

Follows home-assistant/core#30334

Pull request in home-assistant (if applicable): home-assistant/core#30334

Checklist:

  • Branch: next is for changes and new documentation that will go public with the next Home Assistant release. Fixes, changes and adjustments for the current release should be created against current.
  • The documentation follows the standards.

@probot-home-assistant probot-home-assistant Bot added has-parent This PR has a parent PR in another repo next This PR goes into the next branch labels Jan 11, 2020
@Quentame Quentame force-pushed the freebox/config-flow branch 2 times, most recently from 6ca3a5e to 5f59e72 Compare January 11, 2020 18:00
Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Quentame!

Left a couple of comments/suggestions. Could you please take a look? Thanks! 👍

Comment thread source/_integrations/freebox.markdown Outdated
Comment thread source/_integrations/freebox.markdown Outdated
Comment thread source/_integrations/freebox.markdown Outdated
Comment thread source/_integrations/freebox.markdown Outdated
<div class='note warning'>

If you change your Freebox router for a new one, you need to delete the `freebox.conf` file located in your Home Assistant configuration directory to make the association again.
If you change your Freebox router for a new one, go into your Home Assistant configuration `.storage` folder and delete the "freebox" folder, then add the component again.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
If you change your Freebox router for a new one, go into your Home Assistant configuration `.storage` folder and delete the "freebox" folder, then add the component again.
If you change your Freebox router for a new one, go into your Home Assistant configuration `.storage` folder and delete the "freebox" folder, then add the integration again.

Besides, this sounds weird. Shouldn't the user just delete the integration via the UI and re-add it? The cleanup of the storage folder can be done automatically as part of the unload procedure right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That would be nice actually. I didn't know something was planned to clean up the storage folder.

Will see (and for the iCloud integration), not in the PR for now, will see and update here as soon as it's get coded 😉.

Maybe in a new PR perhaps, there is already a lot in this one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

? I'm not saying it happens automatically, but there is an integration unload method that allows you do those things right?

I think we should never instruct the user to go into their .storage folder.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I understood not automatically.

I thought the helpers.storage.Store has a function that can clear itself (depending on the STORAGE_KEY of course) and I could call this function on the async_unload_entry of the integration.

I completely agree that the user shouldn't go to the .storage folder (there is a reason why it is invisible).

In fact I've updated the location here but during my development and test, I didn't need to remove the conf file at all. Before, the token file was freebox.conf, now it's f"{freebox_dir.path}/{slugify(self._host)}.conf" : since the host will change with a new Freebox, I don't think this note is still necessary.

@frenck frenck added new-feature This PR adds documentation for a new Home Assistant feature to an existing integration in-progress This PR/Issue is currently being worked on labels Jan 12, 2020
@Quentame Quentame removed their assignment Jan 12, 2020
@Quentame Quentame requested a review from frenck January 12, 2020 10:20
frenck
frenck previously approved these changes Jan 12, 2020
Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

✅ Approved. Can be merged as soon as the parent PR gets merged.

@frenck frenck added awaits-parent Awaits the merge of an parent PR and removed in-progress This PR/Issue is currently being worked on labels Jan 12, 2020
@Quentame Quentame force-pushed the freebox/config-flow branch from 0e6ce83 to 5133e3c Compare January 27, 2020 20:52
@Quentame
Copy link
Copy Markdown
Member Author

Rebased to trigger the new pipeline.

frenck
frenck previously approved these changes Jan 28, 2020
@Quentame Quentame force-pushed the freebox/config-flow branch from 79370ca to a09d133 Compare March 11, 2020 19:52
@Quentame
Copy link
Copy Markdown
Member Author

Parent PR ready home-assistant/core#30334 🎉

@Quentame Quentame changed the title Add config flow to Freebox Refactor Freebox : add config flow + temperature sensor + signal dispatch Mar 11, 2020
@probot-home-assistant probot-home-assistant Bot added the parent-merged The parent PR has been merged already label Mar 11, 2020
@frenck frenck merged commit 10e724f into home-assistant:next Mar 12, 2020
@probot-home-assistant probot-home-assistant Bot removed the awaits-parent Awaits the merge of an parent PR label Mar 12, 2020
@probot-home-assistant probot-home-assistant Bot removed the parent-merged The parent PR has been merged already label Mar 12, 2020
@Quentame Quentame deleted the freebox/config-flow branch March 12, 2020 10:25
guillempages pushed a commit to guillempages/home-assistant.io that referenced this pull request Mar 21, 2020
…-assistant#11712)

* Add config flow to Freebox

* ✏️ Tweak

* Review from frenck

Co-authored-by: Klaas Schoute <klaas_schoute@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-parent This PR has a parent PR in another repo new-feature This PR adds documentation for a new Home Assistant feature to an existing integration next This PR goes into the next branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants