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

[climate] Bugfix/Tweak honeywell migration #25369

Merged
merged 5 commits into from Jul 23, 2019

Conversation

@zxdavb
Copy link
Contributor

commented Jul 21, 2019

Breaking Change:

The entity_id is changed - now set to a sets a unique/persistent device ID.

This has been removed - there is no breaking change now.

Description:

Changes:

  • Changes consistent with home-assistant/architecture#264
  • DEBUG logging now includes all state data (_LOGGER.debug("latestData = %s ", device._data))
  • Adds demand response state to device_state_attributes as per #25311, and feature request
  • sets the correct outer bounds for min/max temps
  • sets a unique/persistent entity_id
  • bugfix as below issue(s)
  • Minor refactoring

FWIW, I think the demand response data should have been there in the first place, and is not an enhancement. It adds these lines of code to device_state_attributes:

if self._device._data['drData']:
    data['dr_phase'] = self._device._data['drData'].get('Phase')

Related issue (if applicable): fixes #25412

Pull request with documentation for home-assistant.io (if applicable): N/A

Example entry for configuration.yaml (if applicable):

N/A - no changes

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@zxdavb zxdavb changed the title Tweak honeywell migration [climate] Tweak honeywell migration Jul 21, 2019

@zxdavb zxdavb marked this pull request as ready for review Jul 21, 2019

@pvizeli
Copy link
Member

left a comment

You should also fix the upstream library to remove pylint: disable=protected-access. To disable this is a temporary solution and nothing that should be for a core integration.

@zxdavb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

You should also fix the upstream library to remove pylint: disable=protected-access.

I try to remove/reduce these lint hints where possible, such as switching from device._data['uiData' to device.raw_ui_data. In this respect this PR improves the situation dramatically (and I can see where I can take out another).

Unfortunately, the client library is not mine, and has had no changes for >2 years. The owner is around, though...

But in fact, honeywell is nothing to do with me, really, I only got involved because someone stuck Honeywell US and Honeywell International (which uses the evohome client library) together in the same integration & I wanted to pull the international bit out. I don't even have regular access to any Honeywell systems...

Sorry, I have no appetite/time for fixing this client library.

Is it OK to accept this PR as is, given that it is much better than it was?

@fabaff fabaff changed the title [climate] Tweak honeywell migration Tweak honeywell migration Jul 22, 2019

@zxdavb zxdavb changed the title Tweak honeywell migration DONT MERGE - Tweak honeywell migration Jul 23, 2019

@zxdavb zxdavb closed this Jul 23, 2019

@zxdavb zxdavb reopened this Jul 23, 2019

refactor supported_features
add dr_data

delint

simplify code

refactor target_temps, and delinting

delint

fix typo

add min/max temps

delint

refactor target temps

refactor target temps 2

correct typo

tweak

fix potential bug

fix potential bug 2

bugfix entity_id

fix typo

@zxdavb zxdavb changed the title DONT MERGE - Tweak honeywell migration [climate] Bugfix/Tweak honeywell migration Jul 23, 2019

zxdavb added some commits Jul 23, 2019

@zxdavb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@pvizeli OK to go now, thanks.

@balloob balloob added this to the 0.96.4 milestone Jul 23, 2019

@balloob balloob merged commit a1bccb1 into home-assistant:dev Jul 23, 2019

9 checks passed

CI Build #20190723.50 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python35) Tests PyTest Python35 succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA

@zxdavb zxdavb deleted the zxdavb:tweak_honeywell_migration branch Jul 23, 2019

balloob added a commit that referenced this pull request Jul 24, 2019

[climate] Bugfix/Tweak honeywell migration (#25369)
* refactor supported_features

add dr_data

delint

simplify code

refactor target_temps, and delinting

delint

fix typo

add min/max temps

delint

refactor target temps

refactor target temps 2

correct typo

tweak

fix potential bug

fix potential bug 2

bugfix entity_id

fix typo

* remove explicit entity_id

* refactor hvac_action

* refactor hvac_action

* bugfix - HVAC_MODE_HEAT_COOL incorrectly added to hvac_modes
@balloob balloob referenced this pull request Jul 24, 2019

@lock lock bot locked and limited conversation to collaborators Jul 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.