search

nguyer / homeassistant-bond-home

38 stars 15 forks source link

Added support for Device Type from Bond API. #4

Closed bcb2000 closed 4 years ago

bcb2000 commented 4 years ago

Note, in this commit, I needed to change bond.py from bond-home as well as the files in homeassistant-bond-home. It struck me that having these in two separate projects was going to be a bit of a blocker to progress, so I brought bond.py into this project.

nguyer commented 4 years ago

First of all, thank you very much for contributing to this project!

I completely understand needing to change the bond-home library as well. It's a really small library, and I probably would have included it in the component in the first place, except the HA docs recommend against that: https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#5-communication-with-devicesservices

All API specific code has to be part of a third party library hosted on PyPi. Home Assistant should only interact with objects and not make direct calls to the API.

Do you mind making a separate PR with the changes to the bond home python library? I'm happy to review that, merge it, and publish a new version to PyPi. Then you can just require that new version in your changes here.

bcb2000 commented 4 years ago

No problem Nicko, I've done that. I'll come back and update homeassistant-bond-home to use the library a bit later.

nguyer commented 4 years ago

@bcb2000 I just pushed 0.0.5 of the bond-home library with your changes in it, and updated this repo to require it. If you update your PRs here, I'd be happy to merge them.

niemyjski commented 4 years ago

That would be awesome to get all these merged in :)

bcb2000 commented 4 years ago

Hi Nicko

I looked at this for a while this morning. Bear in mind, I'm new to Python, so feeling my way a bit here.

0.0.5 of the bond-home library does not behave the same way as the local bond.py file did. It only exposes the Bond class, not all the constants. I can resolve this locally by placing the entire contents of bond.py in __init__.py in the bond-home library (and removing bond.py)

What's the correct way to achieve this, i.e. make constants available from a library?

nguyer commented 4 years ago

Good point! I'll fix the bond-home library and push a new version. Sorry I didn't catch that, the first time around.

nguyer commented 4 years ago

@bcb2000 I just pushed bond-home 0.0.6 which exports all the constants now. You should be able to include them the same way you are in this PR.

niemyjski commented 4 years ago

Is there anything I can do to help get these pr's merged?

nguyer commented 4 years ago

Closed, due to https://github.com/nguyer/homeassistant-bond-home/pull/11 superseding this PR