core
core copied to clipboard
Update keymitt_ble config_flow.py
Proposed change
PR is a hotfix that ideally needs to be included in a patch release (integration is currently broken)
Update to the config flow to use a new 'is_connected' property from pyMicrobot. Bumped here https://github.com/home-assistant/core/pull/110502
This is therefore PR 2 of 2
Type of change
- [ ] Dependency upgrade
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR is related to issue: https://github.com/home-assistant/core/issues/109742
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests) - [x] 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:
- [x] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest. - [x] New or updated dependencies have been added to
requirements_all.txt.
Updated by runningpython3 -m script.gen_requirements_all. - [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [x] Untested files have been added to
.coveragerc.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
I tried to get away with making pyMicrobot backwards compatible but it didn't work and this PR is still needed to get the integration working again (hence me reopening it)
edit: actually it did work but wasn't released. This PR has therefore become for general code improvements and addressing of late review comments
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
There's a conflict for the config flow now. I think this looks mostly fine otherwise, but I don't see where
self._clientis set in the config flow.
yeah it's not currently working. I didn't expect a review to be honest as it's in draft now but not complaining! I was trying to address this...
"""Given a configured host, will ask the user to press the button to pair."""
errors: dict[str, str] = {}
token = randomid(32)
self._client = MicroBotApiClient(
Should we really create a new client every time this method is called in the same flow?
Originally posted by @MartinHjelmare in https://github.com/home-assistant/core/pull/76575#discussion_r989711205
Should we really create a new client every time this method is called in the same flow?
Actually, in hindsight I think the answer is yes...probably? We are establishing a temporary connection here with a random token to initiate the pairing process and get a real token, which should only happen in the linking step. I don't see how you can definite it anywhere else? (keep in mind that I am by no means any sort of expert with python)
In the link step, add a check if the client attribute is None. If so create the client and store it in the client attribute.
In the link step, add a check if the client attribute is None. If so create the client and store it in the client attribute.
Aaaah it all makes sense now. So simple. Thanks :thumbsup:
Tested and working
You still need to rebase your branch (or merge the new commits from HA/dev into it) to fix the conflict. You should even be able to do that using the GitHub UI now. If you need help, let me know.
You still need to rebase your branch (or merge the new commits from HA/dev into it) to fix the conflict. You should even be able to do that using the GitHub UI now. If you need help, let me know.
That should be done now
I think it's probably a good time to call it a day. I don't know how to fix the tests
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. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!