Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update bt_smarthub component making it compatible with smarthub 2 #31292

Merged
merged 5 commits into from Apr 20, 2020
Merged

Update bt_smarthub component making it compatible with smarthub 2 #31292

merged 5 commits into from Apr 20, 2020

Conversation

leroyshirto
Copy link
Contributor

@leroyshirto leroyshirto commented Jan 29, 2020 •
edited

Proposed change

version 0.2.0 of the btsmarthub_devicelist package makes it compatible with BT's home hub 2 routers. The API of the library has changed. This change makes the component code compatible with the changes to the library.

This change resolves the 404 errors members in the comunity have been experiencing
jxwolstenholme/btsmarthub_devicelist#3
https://community.home-assistant.io/t/has-anyone-had-any-success-using-the-bt-home-hub-5-device-tracker-component-recently/38289/25

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
device_tracker:
  - platform: bt_smarthub

Note: testing this requires a BT Smarthub or Smarthub 2.

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:

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

@homeassistant
Copy link
Contributor

Hi @leroyshirto,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant probot-home-assistant bot added integration: bt_smarthub small-pr PRs with less than 30 lines. labels Jan 29, 2020
@project-bot project-bot bot added this to Needs review in Dev Jan 29, 2020
@probot-home-assistant
Copy link

Hey there @jxwolstenholme, mind taking a look at this pull request as its been labeled with a integration (bt_smarthub) you are listed as a codeowner for? Thanks!

@leroyshirto leroyshirto force-pushed the update-btsmarthub_devicelist-package branch 3 times, most recently from c822b65 to f5e7640 Compare January 31, 2020 06:45
@leroyshirto
Copy link
Contributor Author

@jxwolstenholme I have implemented the changes you suggested in your feedback. Please could you review the technical implementation?

As we are now offering a config option to users I will look at updating the docs to reflect.

@jxwolstenholme
Copy link
Contributor

Looks good. Yes, agreed that the docs should be updated to confirm that Smarthub 2 support is now implemented.

jxwolstenholme
jxwolstenholme approved these changes Feb 2, 2020
View reviewed changes
Copy link
Contributor

@jxwolstenholme jxwolstenholme left a comment

Choose a reason for hiding this comment

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

Approved from my end.

@leroyshirto leroyshirto force-pushed the update-btsmarthub_devicelist-package branch from f5e7640 to f41fce9 Compare February 4, 2020 20:58
@leroyshirto leroyshirto mentioned this pull request Feb 4, 2020
Merged
7 tasks
@leroyshirto leroyshirto marked this pull request as ready for review February 4, 2020 21:13
@leroyshirto leroyshirto mentioned this pull request Feb 4, 2020
Closed
@stale
Copy link

stale bot commented Mar 21, 2020

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.

@stale stale bot added the stale label Mar 21, 2020
frenck
frenck reviewed Mar 22, 2020
View reviewed changes
Copy link
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, @leroyshirto, left 2 small comments.

homeassistant/components/bt_smarthub/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/bt_smarthub/device_tracker.py Outdated Show resolved Hide resolved
@stale stale bot removed the stale label Mar 22, 2020
@leroyshirto leroyshirto force-pushed the update-btsmarthub_devicelist-package branch from 483e3d0 to f41b36d Compare March 30, 2020 11:28
@leroyshirto leroyshirto deleted the update-btsmarthub_devicelist-package branch March 30, 2020 11:33
Dev automation moved this from Needs review to Cancelled Mar 30, 2020
@leroyshirto leroyshirto restored the update-btsmarthub_devicelist-package branch March 30, 2020 11:37
@frenck frenck reopened this Mar 30, 2020
Dev automation moved this from Cancelled to Incoming Mar 30, 2020
MartinHjelmare
MartinHjelmare requested changes Apr 15, 2020
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please rebase on latest dev branch and solve the merge conflict.

homeassistant/components/bt_smarthub/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/bt_smarthub/device_tracker.py Outdated Show resolved Hide resolved
Dev automation moved this from Incoming to Review in progress Apr 15, 2020
@leroyshirto leroyshirto force-pushed the update-btsmarthub_devicelist-package branch 2 times, most recently from c158858 to bc9b6d9 Compare April 15, 2020 10:51
Dev automation moved this from Review in progress to Reviewer approved Apr 15, 2020
MartinHjelmare
MartinHjelmare approved these changes Apr 15, 2020
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MartinHjelmare
Copy link
Member

The CI checks on this page aren't updated properly. The formatting check failed:
https://dev.azure.com/home-assistant/Core/_build/results?buildId=37555&view=logs&j=2fa2c639-d05e-530d-034e-4fb413fcea4e&t=88bd70b9-15d8-56b4-acb9-94f6fcce0e9d

Please run black and commit the changes.

black --fast homeassistant

leroyshirto and others added 5 commits April 15, 2020 16:29
@leroyshirto
0.2.0 of the btsmarthub_devicelist package makes it compatable with B…
5a25949 
…T's home hub 2.

The API has changed in the new version so this change also makes the component code compatible with the changes to the library.
@leroyshirto @frenck
Update homeassistant/components/bt_smarthub/device_tracker.py
3b099ff 
Co-Authored-By: Franck Nijhof <frenck@frenck.nl>
@leroyshirto @frenck
Update homeassistant/components/bt_smarthub/device_tracker.py
16939b8 
Co-Authored-By: Franck Nijhof <frenck@frenck.nl>
@leroyshirto
Remove dep on config in BTSmartHubScanner
b3fe9d8 
This should make BTSmartHubScanner easier to test as you can pass in a mock smarthub_client
@leroyshirto
Black format bt_smarthub
2e4a325
@leroyshirto leroyshirto force-pushed the update-btsmarthub_devicelist-package branch from bc9b6d9 to 2e4a325 Compare April 15, 2020 15:38
@leroyshirto
Copy link
Contributor Author

leroyshirto commented Apr 20, 2020 •
edited

The CI checks on this page aren't updated properly. The formatting check failed:
https://dev.azure.com/home-assistant/Core/_build/results?buildId=37555&view=logs&j=2fa2c639-d05e-530d-034e-4fb413fcea4e&t=88bd70b9-15d8-56b4-acb9-94f6fcce0e9d

Please run black and commit the changes.

black --fast homeassistant

@MartinHjelmare Do we need to re-run Azure Pipelines after I fixed the formatting or does that happen automatically?

@MartinHjelmare
Copy link
Member

A checks have passed now. 👍

MartinHjelmare
MartinHjelmare approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit 6fc517f into home-assistant:dev Apr 20, 2020
Dev automation moved this from Reviewer approved to Done Apr 20, 2020
@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frenck frenck frenck left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@jxwolstenholme jxwolstenholme jxwolstenholme approved these changes

Assignees
No one assigned
Labels
cla-signed integration: bt_smarthub small-pr PRs with less than 30 lines.
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@leroyshirto @homeassistant @jxwolstenholme @MartinHjelmare @frenck