Add Streamlabs Water Monitor #21205
Add Streamlabs Water Monitor #21205
Conversation
ghost
added
the
cpopp
force-pushed
the
streamlabswater
branch
from
ad86dce to
1409e76
Compare
|
Let me know if there is anything else I should address. As far as I'm aware I've addressed all feedback up to this point. |
|
📚Please, update the documentation and open a PR for it (be sure to create a documentation PR against the 🏷 I am adding the 👍 Thanks (times a million!) |
Will do. I had assumed it was fine to just start with the docstrings based on the developer docs, but it sounds like it's not, so when I get some time I'll figure out the docs and open a PR there. |
| ATTR_AWAY_MODE = 'away_mode' | ||
| SERVICE_SET_AWAY_MODE = 'set_away_mode' | ||
| AWAY_MODE_AWAY = 'away' | ||
| AWAY_MODE_HOME = 'home' |
There was a problem hiding this comment.
Those two constants should be named in a clear way according their content.
There was a problem hiding this comment.
Do you have an example of what they should be changed to? I modeled the approach after the nest integration for consistency and this is how it declared those constants.
There was a problem hiding this comment.
why not just HOME = 'home' and AWAY = 'away' as values for something called DEVICE_MODE?
There was a problem hiding this comment.
Could there be more modes than away or home? Why not to make it is_away ?
There was a problem hiding this comment.
The notion of an away mode seemed like something that could eventually be a general concept so I followed the approach used by climate which calls the service set_away_mode. For consistency and ease of search I used the same constant names used in the nest integration. I don't necessarily disagree with the idea that something different might be more optimal, but until someone designs a common architecture to plug in to I thought being consistent with existing integrations and climate was better.
There was a problem hiding this comment.
I agree to use similar const names as in climate. But in climate "away" is binary. There's no AWAY_MODE_AWAY, AWAY_MODE_HOME etc. I mean "away" notion is binary in its essence. Someone/Something is either away or not. And climate implements it accordingly, with turn_away_mode_on() and turn_away_mode_off() and a bool property is_away_mode_on
There was a problem hiding this comment.
@Adminiuga yeah I agree mine looks a bit more like the nest integration than climate in that regard.
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| ATTR_AWAY_MODE = 'away_mode' | ||
| SERVICE_SET_AWAY_MODE = 'set_away_mode' |
There was a problem hiding this comment.
This constant is already present.
There was a problem hiding this comment.
You're referring to the climate constants, right? I opted to not go with those because my device has nothing to do with climate.
|
|
||
| NAME_DAILY_USAGE = "Daily Water" | ||
| NAME_MONTHLY_USAGE = "Monthly Water" | ||
| NAME_YEARLY_USAGE = "Yearly Water" |
There was a problem hiding this comment.
Snake case please
| NAME_YEARLY_USAGE = "Yearly Water" | |
| NAME_YEARLY_USAGE = "yearly_water" |
There was a problem hiding this comment.
These are just used for the names which as far as I could tell in other sensors are typically capitalized with spaces (or accepted freeform as config parameters).
There was a problem hiding this comment.
Correct. For names spaces and capitalization is allowed. Attribute keys should be snake_case underscore
| self._streamlabs_usage_data.update() | ||
|
|
||
|
|
||
| class StreamLabsMonthlyUsage(Entity): |
There was a problem hiding this comment.
StreamLabsMonthlyUsage() and StreamLabsYearlyUsage() should be integrated in StreamLabsDailyUsage. There is a lot of duplicate code. StreamLabsUsage could get the details for a dict/list which describes the different sensors.
There was a problem hiding this comment.
I changed the monthly and yearly classes to extend the daily usage and that got rid of most of what was common.
|
I rebased the changes on the latest dev since some conflicting changes were added since I opened the PR. |
| @@ -0,0 +1,73 @@ | |||
| """Support for Streamlabs Water Monitor Away Mode.""" | |||
There was a problem hiding this comment.
Why not implement this as a switch and get rid of the streamlabswater.set_away_modeservice?
There was a problem hiding this comment.
Two reasons: 1.) When I first started to implement this I considered that but then noticed that would cause it to be controlled along with all other switches in a call to turn_off switches when not specifying a particular entity_id (via stuff a user might have set up programatically or via the one click button in the UI to toggle all switches). This seemed pretty risky to me in that disabling away_mode unintentionally could potentially mean a user's home is not being monitored for water user while gone and a leak would go undetected. So, I thought it was worth making it a separate service so a user could trust it would be operated on independently and integrated in a way.
2.) In general when I surveyed the various switches in home assistant nearly all of them represented turning on or off some physical entity in the real world rather than a conceptual behavior of the entity...so while technically it applicable (it's just a togglable binary value after all) it seemed like it did not fit with the intent of what a switch is.
There was a problem hiding this comment.
You are absolutely right! But if you build it as an alarm_panel it would make sense, arm away and arm home matches the service call & you can show the triggered state instead of the binary sensor.
There was a problem hiding this comment.
Right, that makes sense. Long term I think it would be cool for the away mode to integrate with some presence system supported in the core of home assistant so that it gets managed automatically for the user. For now someone could use the switch template if they wanted it to act like a switch.
There was a problem hiding this comment.
The "integration with presence system" is done via automations like this one:
automation:
trigger:
- platform: zone
entity_id: device_tracker.paulus
zone: zone.home
event: leave
action:
- service: alarm_control_panel.alarm_arm_away
data:
entity_id: alarm_control_panel.streamlabs_waterThe Streamlabs Water component is unable to recover if it is given an invalid API key or location id so this change is to ensure we validate they are correct during setup and return a failure if they are not.
The sensors for the component were not causing an immediate load of data from the API when being set up so there was some lag after startup before values would show up. This change does an explicit update when the sensors are setup to ensure data is viewable immediately after startup.
Dependencies were incorrectly specified using DEPENDS rather than DEPENDENCIES
Remove detailed class docstrings since they're in the documentation, reduce code duplication in sensor classes, and remove periods from the end of log messages.
Codecov Report
@@ Coverage Diff @@
## dev #21205 +/- ##
==========================================
- Coverage 94.29% 93.83% -0.46%
==========================================
Files 458 448 -10
Lines 37203 36529 -674
==========================================
- Hits 35079 34276 -803
- Misses 2124 2253 +129
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## dev #21205 +/- ##
==========================================
- Coverage 94.29% 93.83% -0.46%
==========================================
Files 458 448 -10
Lines 37203 36529 -674
==========================================
- Hits 35079 34276 -803
- Misses 2124 2253 +129
Continue to review full report at Codecov.
|
| from homeassistant.helpers import discovery | ||
| import homeassistant.helpers.config_validation as cv | ||
|
|
||
| REQUIREMENTS = ['streamlabswater==1.0.1'] |
There was a problem hiding this comment.
This constant is no longer used. Please remove.
* Add Streamlabs Water Monitor * Fail Streamlabswater component setup when given invalid parameters The Streamlabs Water component is unable to recover if it is given an invalid API key or location id so this change is to ensure we validate they are correct during setup and return a failure if they are not. * Prime Streamlabswater component sensors so data is available immediately The sensors for the component were not causing an immediate load of data from the API when being set up so there was some lag after startup before values would show up. This change does an explicit update when the sensors are setup to ensure data is viewable immediately after startup. * Switch Streamlabswater logging to use %s for string formatting * Update Streamlabswater component with correct dependencies Dependencies were incorrectly specified using DEPENDS rather than DEPENDENCIES * Streamlabswater pull request feedback Remove detailed class docstrings since they're in the documentation, reduce code duplication in sensor classes, and remove periods from the end of log messages. * Reduce line length in Streamlabswater sensor * Add docstring on Streamlabswater service callback method * Get rid of unnecessary initializers in Streamlabswater sensor * Add manifest file for Streamlabs Water Monitor * Remove unused REQUIREMENTS
Description:
Related issue (if applicable): fixes #
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8733
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: