ReadYourMeter Pro integration #85986

OnFreund · 2023-01-16T06:13:15Z

Proposed change

A new integration for the Read Your Meter Pro service, which monitors your water usage. Starting this with a single sensor reflecting the last read, and will follow up with additional sensors in the future.

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)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add RYM Pro integration documentation home-assistant.io#25845

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:

Documentation added/updated for www.home-assistant.io

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

homeassistant/components/rympro/manifest.json

homeassistant/components/rympro/__init__.py

homeassistant/components/rympro/sensor.py

homeassistant/components/rympro/config_flow.py

thecode

Also tested locally with my meter (including changing the password to trigger reauth), great work!

frenck

Please address these in a new PR.

frenck · 2023-01-24T20:44:32Z

homeassistant/components/rympro/__init__.py

+    return unload_ok
+
+
+class RymProDataUpdateCoordinator(DataUpdateCoordinator):


Coordinators should go into their own modules, in this case, move it into coordinator.py.

frenck · 2023-01-24T20:47:29Z

homeassistant/components/rympro/config_flow.py

+                self._reauth_entry,
+                title=title,
+                data=data,
+                unique_id=info[CONF_UNIQUE_ID],


This can't change, right? Why is it updated?

As a matter of fact, what in this re-auth process guards that the right account is re-authed?

frenck · 2023-01-24T20:49:08Z

homeassistant/components/rympro/sensor.py

+        [
+            RymProSensor(coordinator, meter_id, meter["read"], config_entry.entry_id)
+            for meter_id, meter in coordinator.data.items()
+        ]


async_add_entities accepts a generator, you can remove the list comprehension here.

frenck · 2023-01-24T20:49:27Z

homeassistant/components/rympro/sensor.py

+class RymProSensor(CoordinatorEntity[RymProDataUpdateCoordinator], SensorEntity):
+    """Sensor for RymPro meters."""
+
+    _attr_has_entity_name = True


This seems to be incorrect, as there are conditions not met for this to be set.

Will change the name to sentence case. Other than that, is there any other condition that's not met?

That's about it as it seems.

frenck · 2023-01-24T20:49:49Z

homeassistant/components/rympro/sensor.py

+    """Sensor for RymPro meters."""
+
+    _attr_has_entity_name = True
+    _attr_name = "Last Read"


This must be following sentence case styling to be able to set has_entity_name.

Additionally, it is a bit of an odd name, it would suggest a date of when it was last read? But I guess it is water usage? Would that be a better name?

The read here is in present tense, as in the meter read. Common usage, and how the service refers to it, but maybe total consumption? As opposed to some periodic consumption (e.g. daily, monthly), which I plan on adding in future PRs.

We don't describe entities on what they do.. we describe what they are/represent. They represent consumption or usage in this case.

homeassistant/components/rympro/sensor.py

frenck · 2023-01-24T20:52:44Z

homeassistant/components/rympro/sensor.py

+        """Initialize sensor."""
+        super().__init__(coordinator)
+        self._meter_id = meter_id
+        self._entity_registry: er.EntityRegistry | None = None


This is unused?

frenck · 2023-01-24T20:54:49Z

homeassistant/components/rympro/sensor.py

+    @callback
+    def _handle_coordinator_update(self) -> None:
+        """Handle updated data from the coordinator."""
+        self._attr_native_value = self.coordinator.data[self._meter_id]["read"]
+        self.async_write_ha_state()


There is no need to handle it like this. The coordinator already has the data and there are no calculations involved here. Implement the native_value property method instead.

frenck · 2023-01-24T20:56:00Z

tests/components/rympro/test_config_flow.py

+    assert result2["type"] == FlowResultType.FORM
+    assert result2["errors"] == {"base": error}


This test is incomplete.

For example, if authentication is incorrect, a user can recover from it. Please make the test complete until the config entry is created (to ensure there are no side-effects from recovering from the error handling).

OnFreund

Thanks @frenck!
I will address these in a new PR ASAP. Left some clarification comments/questions in the meantime.

OnFreund · 2023-01-25T06:21:28Z

homeassistant/components/rympro/sensor.py

+class RymProSensor(CoordinatorEntity[RymProDataUpdateCoordinator], SensorEntity):
+    """Sensor for RymPro meters."""
+
+    _attr_has_entity_name = True


Will change the name to sentence case. Other than that, is there any other condition that's not met?

OnFreund · 2023-01-25T06:23:24Z

homeassistant/components/rympro/sensor.py

+    """Sensor for RymPro meters."""
+
+    _attr_has_entity_name = True
+    _attr_name = "Last Read"


The read here is in present tense, as in the meter read. Common usage, and how the service refers to it, but maybe total consumption? As opposed to some periodic consumption (e.g. daily, monthly), which I plan on adding in future PRs.

homeassistant/components/rympro/sensor.py

MartinHjelmare · 2023-01-25T06:16:47Z

homeassistant/components/rympro/__init__.py

+    except UnauthorizedError:
+        try:
+            token = await rympro.login(data[CONF_EMAIL], data[CONF_PASSWORD], "ha")
+            hass.config_entries.async_update_entry(


Please only wrap the line that can raise in the try... except block. Move this line down.

thecode · 2023-01-26T18:12:04Z

Follow up PR: #86734

home-assistant bot added cla-signed has-tests new-integration integration: rympro labels Jan 16, 2023

ReadYourMeter Pro integration

9a79b49

OnFreund force-pushed the rympro_integration branch from 2fec62a to 9a79b49 Compare January 16, 2023 13:23

Add __init__.py to .coveragerc

cb588e7

OnFreund mentioned this pull request Jan 19, 2023

Add RYM Pro integration documentation home-assistant/home-assistant.io#25845

Merged

8 tasks

thecode requested changes Jan 24, 2023

View reviewed changes

Address code review comments

96e9155

OnFreund mentioned this pull request Jan 24, 2023

Rympro integration icon home-assistant/brands#4068

Merged

11 tasks

OnFreund requested a review from thecode January 24, 2023 19:52

More code review comments

cc58cf6

thecode approved these changes Jan 24, 2023

View reviewed changes

thecode merged commit 2c9e8ad into home-assistant:dev Jan 24, 2023

frenck reviewed Jan 24, 2023

View reviewed changes

OnFreund commented Jan 25, 2023

View reviewed changes

OnFreund deleted the rympro_integration branch January 25, 2023 06:29

MartinHjelmare reviewed Jan 25, 2023

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReadYourMeter Pro integration #85986

ReadYourMeter Pro integration #85986

OnFreund commented Jan 16, 2023 •

edited

Loading

thecode left a comment

frenck left a comment

frenck Jan 24, 2023

frenck Jan 24, 2023 •

edited

Loading

frenck Jan 24, 2023

frenck Jan 24, 2023

OnFreund Jan 25, 2023

frenck Jan 25, 2023

frenck Jan 24, 2023

frenck Jan 24, 2023

OnFreund Jan 25, 2023

frenck Jan 25, 2023

frenck Jan 24, 2023

frenck Jan 24, 2023

frenck Jan 24, 2023 •

edited

Loading

OnFreund left a comment

OnFreund Jan 25, 2023

OnFreund Jan 25, 2023

MartinHjelmare Jan 25, 2023

thecode commented Jan 26, 2023

		return unload_ok


		class RymProDataUpdateCoordinator(DataUpdateCoordinator):

		assert result2["type"] == FlowResultType.FORM
		assert result2["errors"] == {"base": error}

ReadYourMeter Pro integration #85986

ReadYourMeter Pro integration #85986

Conversation

OnFreund commented Jan 16, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

thecode left a comment

Choose a reason for hiding this comment

frenck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenck Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenck Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

OnFreund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecode commented Jan 26, 2023

OnFreund commented Jan 16, 2023 •

edited

Loading

frenck Jan 24, 2023 •

edited

Loading

frenck Jan 24, 2023 •

edited

Loading