Config flow for Velux integration by bramstroker · Pull Request #49348 · home-assistant/core

bramstroker · 2021-04-17T15:35:20Z

Breaking change

Proposed change

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

To help with the load of incoming pull requests:

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

iMicknl · 2021-04-18T10:39:25Z

@bramstroker are you aware that there is another PR open (#42773) with the same functionality? I see it is still limited, but perhaps there are other improvements that can help your PR.

iMicknl

Just had a quick look, happy to give it a more extensive review later on.

homeassistant/components/velux/__init__.py

homeassistant/components/velux/config_flow.py

homeassistant/components/velux/translations/en.json

bramstroker · 2021-04-18T11:00:00Z

@bramstroker are you aware that there is another PR open (#42773) with the same functionality? I see it is still limited, but perhaps there are other improvements that can help your PR.

Ik was not aware of the other PR @iMicknl. It contains mostly the same functionality, but also support for tilting covers. However the other PR seems quite stale tot me atm. Not sure what the best approach would be as I am a new contributer to home assistent.
Close mine and wait for the other PR to progress? Or continue my development in this PR to get this mergeable asap?

Thanks for your review BTW. This is useful anyway.

iMicknl · 2021-04-18T11:03:24Z

Close mine and wait for the other PR to progress? Or continue my development in this PR to get this mergeable asap?

To be honest, I wouldn't know either. If the other PR is stale and not in development anymore, it would make sense to see if you could use the best of both PR's 👍🏻.

It can take a while until a PR is merged due to the huge backlog, thus this could also be the reason it is marked as stale. Perhaps you could discuss this with the author of the other PR? Or wait until a core member chimes in.

homeassistant/components/velux/config_flow.py

Co-authored-by: J. Nick Koston <nick@koston.org>

…ore into feature/velux-config-flow2

… the global hass object, so we can have multiple instances

bramstroker · 2021-05-16T18:46:56Z

Alle review remarks are resolved. Could you guys please have another look?
Maybe it's a good idea that I also merge the branch for velux light support #49338 into this one, so it can be one PR?
I'm pretty sure there will be merge conflicts, so better to fix them now.
What do you think?

mtdcr

Handling connection failures as described in my inline comment would be nice, but as it doesn't introduce a regression, I consider it optional.

mtdcr · 2021-06-04T12:16:57Z

homeassistant/components/velux/__init__.py

-        await hass.data[DATA_VELUX].async_start()
+        veluxModule = VeluxModule(hass, config_entry)
+        veluxModule.setup()
+        await veluxModule.async_start()


async_start() may fail (see #34844), in which case you should raise ConfigEntryNotReady. That would really resolve #34844, I guess. The issue was closed due to inactivity.

mtdcr · 2021-06-04T12:21:36Z

homeassistant/components/velux/strings.json

+      "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]"
+    }
+  }
+}


No newline at end of file.

mtdcr · 2021-06-04T12:29:34Z

Maybe it's a good idea that I also merge the branch for velux light support #49338 into this one, so it can be one PR?
I'm pretty sure there will be merge conflicts, so better to fix them now.

I wouldn't recommend adding more complexity to this PR. Conflicts can still get resolved once one of the PRs got merged.

github-actions · 2021-07-04T13:06:56Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

First draft implementing config flow for velux

c1dc20f

homeassistant added the cla-signed label Apr 17, 2021

probot-home-assistant bot added has-tests integration: velux labels Apr 17, 2021

bramstroker added 4 commits April 17, 2021 18:18

Rename config flow, add string and translations

45fc39c

Import configuration from yaml

0cf4ece

Fix linting

b0b09dc

Fix config is imported only once for the same host

cd98275

iMicknl reviewed Apr 18, 2021

View reviewed changes

bdraco reviewed May 12, 2021

View reviewed changes

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

bramstroker and others added 10 commits May 16, 2021 19:11

Update homeassistant/components/velux/config_flow.py

eb3a78e

Co-authored-by: J. Nick Koston <nick@koston.org>

Remove translation files from git

17c7e42

Merge branch 'feature/velux-config-flow2' of github.com:bramstroker/c…

2400cf2

…ore into feature/velux-config-flow2

Fix typo

e937327

Extend exception handling

702ded2

Extend exception handling

9be4604

Remove unused declaration

0c5a66d

Remove unnessary constant. Register a veluxModule per config entry in…

b5f294a

… the global hass object, so we can have multiple instances

Re-add config schema

8e041d5

Prevent setting up integration for same host twice

6f87d8e

bramstroker marked this pull request as ready for review May 16, 2021 18:47

mtdcr approved these changes Jun 4, 2021

View reviewed changes

github-actions bot added the stale label Jul 4, 2021

github-actions bot closed this Jul 11, 2021

github-actions bot locked and limited conversation to collaborators Jul 12, 2021

Uh oh!

Conversation

bramstroker commented Apr 17, 2021 • edited 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!

iMicknl commented Apr 18, 2021

Uh oh!

iMicknl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bramstroker commented Apr 18, 2021

Uh oh!

iMicknl commented Apr 18, 2021

Uh oh!

Uh oh!

bramstroker commented May 16, 2021

Uh oh!

mtdcr left a comment

Choose a reason for hiding this comment

Uh oh!

mtdcr Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

mtdcr Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

mtdcr commented Jun 4, 2021

Uh oh!

github-actions bot commented Jul 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bramstroker commented Apr 17, 2021 •

edited

Loading

Example entry for `configuration.yaml`: