Skip to content

Update plugwise to async and config_flow sensor part#36219

Merged
MartinHjelmare merged 39 commits intohome-assistant:devfrom
plugwise:plugwise-async-platform-33691-add-sensor
Jun 1, 2020
Merged

Update plugwise to async and config_flow sensor part#36219
MartinHjelmare merged 39 commits intohome-assistant:devfrom
plugwise:plugwise-async-platform-33691-add-sensor

Conversation

@CoMPaTech
Copy link
Copy Markdown
Member

@CoMPaTech CoMPaTech commented May 28, 2020

Breaking change

See #33691 as 'parent'-PR. This PR is the first of three 'child'-PRs for the parent. It adds the sensor platform for feature-compatibility with regard to the breaking changes of the parent PR for current 'Plugwise Anna' climate devices.

Proposed change

Provide for sensors for both climate and non-climate Plugwise components. Moving some attributes (i.e. outdoor_temperature) to sensor as indicated on previous PRs by reviewers.

This PR is awaiting the parent PR to be merged for changes to __init__.py (hence the patchfile).

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 accompanies #33691 to provide comparible functionality for climate devices (i.e. climate and sensor). Addition of this sensor PR also allows DSMR devices. Two more child PRs are following for binary_sensor (also for climate setups) and switch (for power-controle within climate).

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][dev-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:

  • Documentation added/updated for [www.home-assistant.io][docs-repository]

If the code communicates with devices, web services, or third-party tools:

  • The [manifest file][manifest-docs] 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][quality-scale]:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

Hey there @laetificat, @bouwew, mind taking a look at this pull request as its been labeled with a integration (plugwise) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@CoMPaTech CoMPaTech marked this pull request as ready for review May 28, 2020 11:05
@bouwew
Copy link
Copy Markdown
Contributor

bouwew commented May 28, 2020

My review-comments, provided via Discord, have been included in the code.
So, approved.

Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
@CoMPaTech CoMPaTech force-pushed the plugwise-async-platform-33691-add-sensor branch from 2a2ac71 to 03a274c Compare May 28, 2020 21:24
@CoMPaTech
Copy link
Copy Markdown
Member Author

As indicated in the parent PR - the baseclass took some time and just committed as progress. Will check for further issues and self-review in the morning.

Comment thread homeassistant/components/plugwise/__init__.py Outdated
Comment thread homeassistant/components/plugwise/__init__.py Outdated
Comment thread homeassistant/components/plugwise/__init__.py Outdated
Comment thread homeassistant/components/plugwise/__init__.py Outdated
Comment thread homeassistant/components/plugwise/__init__.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
@CoMPaTech CoMPaTech force-pushed the plugwise-async-platform-33691-add-sensor branch from 606cd88 to c793098 Compare May 30, 2020 08:11
@CoMPaTech
Copy link
Copy Markdown
Member Author

Constants have been adjusted further, we moved the Sensor-childclass, but we'll re-use this on binary_sensor, but that can include from sensor nonetheless.

As discussed, working on test - but that will be a better fit in a different PR.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 30, 2020

There is some more cleanup that can be done to get rid of the return None in the base class.

However given the size of this PR and beta coming up, let’s keep the scope contained as additional cleanup can be done in a new PR when the tests are added.

@CoMPaTech
Copy link
Copy Markdown
Member Author

However given the size of this PR and beta coming up, let’s keep the scope contained as additional cleanup can be done in a new PR when the tests are added.

@bouwew noticed something wrong with the (actual) naming, so we cleaned that up just now. w.r.t. @MartinHjelmare 's comment on the parent PR - cleanup of translations still in this PR or also in a forthcoming one?

Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Copy link
Copy Markdown
Member

@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.

Much cleaner. Great work. 👍

Everything still working ?

@CoMPaTech
Copy link
Copy Markdown
Member Author

Works on my setup and when I run the tests against it - @bouwew was still testing some final parts.

Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py Outdated
Comment thread homeassistant/components/plugwise/sensor.py
Copy link
Copy Markdown
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.

Looks good!

@CoMPaTech
Copy link
Copy Markdown
Member Author

As part of peer-review: added icon for non-device class auxiliary back in at request of @bouwew

@bouwew
Copy link
Copy Markdown
Contributor

bouwew commented Jun 1, 2020

Icons are missing for sensors that do not have a DEVICE_CLASS.

@CoMPaTech
Copy link
Copy Markdown
Member Author

Note: if we only take climate and sensor to beta/release we have to modify the doc-PR to reflect that 'plugs' (switch) and the domestic-how-water and secondary heater states (binary_sensor) will be available in a later release - there is no harm done to existing integration (Anna) users, just less functionality of the bat for (Adam) users.

@CoMPaTech
Copy link
Copy Markdown
Member Author

@MartinHjelmare nothing needed to be done anymore for this #33691 (comment) right?

@MartinHjelmare
Copy link
Copy Markdown
Member

No, not now. 👍

@MartinHjelmare
Copy link
Copy Markdown
Member

Ping here when we should merge.

@bouwew
Copy link
Copy Markdown
Contributor

bouwew commented Jun 1, 2020

@MartinHjelmare from @CoMPaTech and me, yes, please merge.
We'll continue working on the two remaining PR's for binary_sensor and switch.

@MartinHjelmare
Copy link
Copy Markdown
Member

Test failure is unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants