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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lyric integration #31387

Closed
wants to merge 10 commits into from
Closed

Conversation

shellster
Copy link

Breaking change

Proposed change

This is my attempt to resubmit a PR to add Honeywell Lyric T5-T6 support. I've included the changes requested from the previous attempt to merge, including getting the original author @bramkragten to merge some changes to his support library and bump the pip library, and then removing my updated version from this pull to make it cleaner.

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:

lyric:
  client_id: wqeioqweuoiewuoadslqsdfkasdiosdu
  client_secret: sadiasduasoiduqwo8erq

Additional information

  • This PR is based off of the original work by @bramkragten, which was prior to the climate change refactor. This code is compliant with that refactor and includes fan support.

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

@project-bot project-bot bot added this to Needs review in Dev Feb 1, 2020
@MartinHjelmare MartinHjelmare changed the title Updating Lyric Plugin Add lyric integration Feb 2, 2020
@jkrall
Copy link
Contributor

jkrall commented Feb 4, 2020

@shellster I was looking into HA support for Honeywell water detectors, and research led me to your PRs here 鈥斅燾an you confirm that this PR brings support for the latest python-lyric up-to-date with HA, but that more work would need to be added on top of this to support water detector devices in HA?

Asking because I see apparent support for these devices in the python-lyric library here: https://github.com/bramkragten/python-lyric/blob/master/lyric/__init__.py#L304

Appreciate any pointers/advice you have here, as I'm considering building upon your work to add support for these myself...

@shellster
Copy link
Author

shellster commented Feb 9, 2020

@jkrall: I actually stripped the water detector code in this last round of updates because it wasn't germane to the temperature code, and I was hoping that streamlining it would help increase the chance that it actually got merged. The water detector code is in an older commit (shellster@12baa93), if you look back. You still will need to add a component inside home-assist, and I haven't really figured out or needed to figure out how that works. Would definitely be interested in PR's to my branch, or it may make more sense to split that into a new module, but happy to entertain the idea. I just have no way to test it.

@shellster
Copy link
Author

To any devs: I don't know how to not fail the codecov/patch check.

@jasondefuria
Copy link

jasondefuria commented Feb 18, 2020

@balloob -it looks like @shellster needs a little help here.

@shellster
Copy link
Author

@frenck I don't know how/where I'm supposed to add documentation. Any help?

@whattheschnell
Copy link

whattheschnell commented Feb 24, 2020

@shellster

The documentation page doesn't exist yet on home-assistant.io It needs its own PR and supporting info to describe to users how they can use the component.

https://home-assistant.io/components/lyric/ or https://home-assistant.io/integrations/lyric/

@shellster
Copy link
Author

I'll work on this this weekend.

@shellster
Copy link
Author

Documentation: home-assistant/home-assistant.io#12217

shellster added 2 commits March 10, 2020 00:37
Push to force script rerun.
Removing added space that I needed to do a new push.
@shellster
Copy link
Author

Problem found upstream. Waiting for another python-lyric pull to land: bramkragten/python-lyric#6

@shellster
Copy link
Author

What do I need to do to get this landed? I don't want to be stuck in a loop repeatedly tending a branch to keep it up to date with dev, if no one is going to land this for me anytime soon.

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.

I've taken a quick look, made the first small comment, scrolled through it a bit and noticed the configurator. That should actually not be used anymore. Since a large part of the code relies on it, I've left this initial review with just that comment.

Let's migrate it to a config flow before looking at it further. 馃憤

Comment on lines +3 to +5

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/lyric/
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these additional lines. Just the first line is OK.


hass.http.register_view(LyricAuthenticateView(lyric))

_CONFIGURING["lyric"] = configurator.request_config(
Copy link
Member

Choose a reason for hiding this comment

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

@bramkragten
Copy link
Member

The other PR #23015 was a long way and had a config flow, might want to check that one out, or use it as a starting point.

@stale
Copy link

stale bot commented Apr 23, 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 Apr 23, 2020
@stale stale bot closed this Apr 30, 2020
Dev automation moved this from Needs review to Cancelled Apr 30, 2020
@lock lock bot locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

7 participants