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.

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

Add new integration for Bond hub #37477

Merged
merged 22 commits into from
Jul 6, 2020
Merged

Conversation

prystupa
Copy link
Contributor

@prystupa prystupa commented Jul 4, 2020

Proposed change

Add new integration for Bond hub

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

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

@ctalkington
Copy link
Contributor

this is looking pretty good. do you know if the device is discoverable via SSDP or mDNS/zerocnf?

@ctalkington
Copy link
Contributor

also, i see you did covers but do you intend to handle some other uses in followup PR

@prystupa
Copy link
Contributor Author

prystupa commented Jul 5, 2020

also, i see you did covers but do you intend to handle some other uses in followup PR

Yes, I do want to handle other domains after covers. As suggested, keeping first PR to one domain and relatively small

@prystupa
Copy link
Contributor Author

prystupa commented Jul 5, 2020

this is looking pretty good. do you know if the device is discoverable via SSDP or mDNS/zerocnf?

As far as I can find, it does not support SSDP. It does however mention mDNS support, though I can't find any specific documentation. I am happy to add this support in the follow up PR, if I can crack it.

prystupa and others added 2 commits July 5, 2020 15:13
adding async_unload_entry per PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
prystupa and others added 2 commits July 5, 2020 15:26
add a unit for unloading per PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
add unit test for unload per PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
prystupa and others added 4 commits July 5, 2020 15:35
PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
PR review suggestion

Co-authored-by: Chris Talkington <[email protected]>
prystupa and others added 2 commits July 5, 2020 15:45
PR suggestion

Co-authored-by: Chris Talkington <[email protected]>
PR suggestion

Co-authored-by: Chris Talkington <[email protected]>
prystupa and others added 2 commits July 5, 2020 15:46
PR suggestion

Co-authored-by: Chris Talkington <[email protected]>
PR suggestion

Co-authored-by: Chris Talkington <[email protected]>
@ctalkington
Copy link
Contributor

sorry for the format issues, github suggestions like to include whitespace and dont have same same format checkers as a full dev suite would.

@ctalkington
Copy link
Contributor

for future reference, the bond mdns name appears to be: _bond._tcp

http://docs-local.appbond.com/#section/Getting-Started/Finding-the-Bond-IP

@prystupa prystupa requested a review from ctalkington July 5, 2020 20:44
prystupa and others added 2 commits July 5, 2020 20:44
add per PR review feedback

Co-authored-by: Chris Talkington <[email protected]>
remove per PR review feedback

Co-authored-by: Chris Talkington <[email protected]>
Copy link
Contributor

@ctalkington ctalkington left a comment

Choose a reason for hiding this comment

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

this honestly all looks pretty well done and youve been receptive to so im going to approve with the caveat that someone who is more familiar with core (this is only my second bigger review) may request a few tweaks post merge. just awaiting tests to pass

fix unit test

Co-authored-by: Chris Talkington <[email protected]>
@ctalkington ctalkington merged commit 9b77e16 into home-assistant:dev Jul 6, 2020
@prystupa prystupa deleted the bond branch July 6, 2020 01:20
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.

Nice! Please address the comments in a new PR. Thanks!

@ViolentPoo
Copy link

ViolentPoo commented Jul 9, 2020

An integration for this device had already been made which has support for all the features. We have been working on getting it into hacs but I’m sure the code owner wouldn’t mind merging here instead https://github.com/nguyer/homeassistant-bond-home

Since this is a fork of his work

@niemyjski
Copy link

I'm a maintainer on that project, did anyone even look at this before starting this? Would be good to merge efforts.

@ctalkington
Copy link
Contributor

it seems sometimes we work in silos. it was not intentional to ignore your custom component when merging this, simply didnt know it existed in the reviewers context. I hope everyone can work together to get the best coverage in core. let me know if you need help getting reviews as im trying to help on any followup work to integrations that ive approved.

@sassa4ras
Copy link

Given that the source for this component is still actively maintained and being updated and this was forked from that project it seems imprudent to merge this until they can be brought to parity.

There are currently a few pull requests on the original pre-forked source that would increase this integrations functionality (eg reversing fan speed) and silly to have concurrent projects going.

@ctalkington
Copy link
Contributor

custom components can still be used in place of the core versions. also, are we sure this is a true fork? i know it uses the same python library but did the component code actually come directly from the custom component? i will say that just going through core review process makes it slightly different in its own way.

I do hope that efforts can be combined and feature parity is reached in future but i dont think its a show stopper in itself. Thats just my opinion though.

@sassa4ras
Copy link

Reviewing the code it does look like a fork of the custom component, but the code is cleaner and it looks like it's already implementing direction and other features still in progress with the custom component. As a user who has contributed nothing to either project, I'm not looking to cause frustration - Just want to make sure that people aren't working redundantly.

@prystupa - seems like it would be good to talk with @niemyjski and see if there are features worth merging or if at this point it's best to just abandon the custom component and divert efforts to this project.

@prystupa
Copy link
Contributor Author

Thanks, @sassa4ras. I did reach out to @niemyjski when I first head of the custom component project (which unfortunately I was not aware when I started contributing to official HA repo). I believe we are on the same page re: future of this integration. We all want the best possible product and best possible user experience.

@ViolentPoo
Copy link

are we sure this is a true fork? i know it uses the same python library but did the component code actually come directly from the custom component?

https://github.com/prystupa/bond-home/network/members looks to be a true fork to me. Perhaps the code owners should be added correctly for this integration...

sorry, im not trying to cause any problems but I would hate for the team that developed this to not be credited properly

@MartinHjelmare
Copy link
Member

Please don't use this merged PR for discussion. Open an issue if there's a problem. Anyone is welcome to contribute. If someone wants to become a code owner they can open a PR for consideration.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants