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

Config flow for harmony #32919

Merged
merged 8 commits into from
Mar 19, 2020
Merged

Config flow for harmony #32919

merged 8 commits into from
Mar 19, 2020

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Mar 17, 2020

Breaking change

As this has been migrated to a ssdp discovery config flow, yaml configuration now requires a host/ip to be configured.

Proposed change

Config flow for harmony

  • Fixes unique ids when using XMPP

  • Removes 'port' option since the underlying library doesn't consume it

Co-authored-by: Bram Kragten mail@bramkragten.nl

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

Additional information

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

@probot-home-assistant
Copy link

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

Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

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

Good job! There is no test covering discovery, which should be covered.

homeassistant/components/harmony/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Outdated Show resolved Hide resolved
homeassistant/components/harmony/config_flow.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2020

CI failure is not related

homeassistant/components/zone/__init__.py:240: error: unused 'type: ignore' comment

Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2020

Looks good to me

Thanks for the review. Much appreciated!

@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2020

@bramkragten Thanks for the double check on this. One step closer to yaml zero

@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2020

Repush for ci

@bdraco bdraco merged commit d33a3ca into home-assistant:dev Mar 19, 2020
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 address the comments in a new PR.

homeassistant/components/harmony/__init__.py Show resolved Hide resolved
homeassistant/components/harmony/__init__.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/remote.py Show resolved Hide resolved
homeassistant/components/harmony/strings.json Show resolved Hide resolved
Copy link
Member Author

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

bdraco added a commit to bdraco/home-assistant that referenced this pull request Mar 19, 2020
* Address followup review comments from home-assistant#32919
bdraco added a commit to bdraco/home-assistant that referenced this pull request Mar 19, 2020
* Address followup review comments from home-assistant#32919
bdraco added a commit to bdraco/home-assistant that referenced this pull request Mar 19, 2020
* Address followup review comments from home-assistant#32919
@bdraco bdraco mentioned this pull request Mar 19, 2020
20 tasks
bdraco added a commit that referenced this pull request Mar 20, 2020
* Harmony config flow improvements

* Address followup review comments from #32919

* pylint -- catching my naming error

* remove leftovers from refactor
balloob pushed a commit that referenced this pull request Mar 20, 2020
* Update powerwall unique id

* Fix somfy optimistic mode when missing in conf (#32995)

* Fix optimistic mode when missing in conf #32971

* Ease code using a default value

* Client id and secret are now inclusive

* Bump aiohomekit to fix Insignia NS-CH1XGO8 and Lennox S30 (#33014)

* Axis - Fix char in stream url (#33004)

* An unwanted character had found its way into a stream string, reverting f-string work to remove duplication of code and improve readability

* Fix failing tests

* deCONZ - Add support for Senic and Gira Friends of Hue remote… (#33022)

* Update the test

* Harmony config flow improvements (#33018)

* Harmony config flow improvements

* Address followup review comments from #32919

* pylint -- catching my naming error

* remove leftovers from refactor

* Update powerwall unique id

* Update the test

Co-authored-by: tetienne <thibaut@etienne.pw>
Co-authored-by: Jc2k <john.carr@unrouted.co.uk>
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
@lock lock bot locked and limited conversation to collaborators Mar 21, 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.

Logitech Harmony Integration not working on new install
4 participants