core icon indicating copy to clipboard operation
core copied to clipboard

Update keymitt_ble config_flow.py

Open spycle opened this issue 1 year ago • 1 comments

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:

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 running python3 -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:

spycle avatar Feb 13 '24 21:02 spycle

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

spycle avatar Feb 26 '24 23:02 spycle

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Feb 28 '24 21:02 home-assistant[bot]

There's a conflict for the config flow now. I think this looks mostly fine otherwise, but I don't see where self._client is 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

spycle avatar Mar 02 '24 00:03 spycle

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)

spycle avatar Mar 02 '24 13:03 spycle

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.

MartinHjelmare avatar Mar 03 '24 09:03 MartinHjelmare

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

spycle avatar Mar 03 '24 12:03 spycle

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.

TheJulianJES avatar Mar 03 '24 20:03 TheJulianJES

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

spycle avatar Mar 03 '24 21:03 spycle

I think it's probably a good time to call it a day. I don't know how to fix the tests

spycle avatar Mar 07 '24 22:03 spycle

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!

github-actions[bot] avatar May 08 '24 17:05 github-actions[bot]