Emoncms cleanup #86840

bwduncan · 2023-01-28T14:25:44Z

Breaking change

Proposed change

Tidy up the code a bit and add some of the newer changes in HA. Specifically, this adds unique_id and sets the state_class for all sensors.

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:

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.

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

@Throttle

Unless I'm missing something obvious, it is never used in the EmonCmsData class, which is instead decorated by @Throttle(MIN_TIME_BETWEEN_UPDATES) If this is supposed to work then it is a bug. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

home-assistant · 2023-01-28T14:25:50Z

Hey there @borpin, mind taking a look at this pull request as it has been labeled with an integration (emoncms) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of emoncms can trigger bot actions by commenting:

@home-assistant close Closes the issue.
@home-assistant rename Awesome new title Change the title of the issue.
@home-assistant reopen Reopen the issue.
@home-assistant unassign emoncms Removes the current integration label and assignees on the issue, add the integration domain after the command.

- apikey is the only parameter, so just create the parameters once at init. - Don't squash the exception when logging it, use logger.exception - Use req.raise_for_status() instead of checking status code manually - Put req.json() in the try block Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

emoncms.org allows Keep-Alive so use a requests.Session object which allows urllib3 connection pooling. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Use _attr_name, _attr_native_unit_of_measurement and _attr_native_value. Convert the _identifier into a _attr_unique_id Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

It always takes the same elements of a dict, so just pass the dict. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

native_value is set twice, so deduplicate the code Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Set state_class even if the unit of measurement is not recognised, since the value will certainly be numeric or null. Emoncms can only return numeric values or null. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

homeassistant/components/emoncms/sensor.py

borpin · 2023-01-28T21:20:24Z

Looks good to me.

frenck · 2023-01-30T10:15:28Z

The CI is failing, please make sure the build passes.

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

bwduncan · 2023-01-30T11:14:46Z

Sorry about that, the build is now passing. Although I'm not sure the resulting code is "better" 😆

epenet · 2023-01-31T14:47:47Z

homeassistant/components/emoncms/sensor.py


    if value_template is not None:
        value_template.hass = hass

-    data = EmonCmsData(hass, url, apikey, interval)
+    assert url is not None and apikey is not None  # Keep mypy happy


If they are compulsory, then it should be using config[key] not config.get(key)

That's a really good point. I had forgotten I was looking at a dict, rather than some opaque data class 😆 They certainly are compulsory, and marked as Required in the schema.

I'd be also worried about this PR getting bigger and bigger.

Adding type hints can usually be a single PR.

Adding unique_id probably can be also it's own PR

and maybe the state_class in a separate PR.

OK, if that is the convention. It seems like there will be a lot of admin overheard for adding one line of code at a time but if that's the standard I am happy to comply.

I don't have the time to pull it all apart this week though. Maybe next week.

It's not compulsory - but it makes reviewing ssssoooo much easier if the PR is smaller.
It also means that if there is a difference of opinion on one point then the other points can be merged anyway.

OK yes I see your point. I guess my view was that a lot of this is just tidying things up and doesn't really improve the user experience, so it makes sense to carry it along with features rather than bothering reviewers to look through multiple PRs.

I am certainly expecting to move the integration to use a config flow in a separate PR.

Actually if you look at the instructions when making a new PR:

If your PR requires multiple boxes to be checked, you'll most likely need to split it into multiple PRs. This makes things easier and faster to code review.

Which ended up with:

Don't get me wrong, I can't say I meet these instructions, many times dependency bump is also a bugfix, etc...

However, code quality are usually easy to split out to a new PR, they are also good candidates for people to review since unlike bug fixes which change the code and understanding for the change, code quality usually try to keep the same functionality and is easier to review.

There is no overhead from creating multiple PRs (@epenet is expert in creating huge number of PRs ❤️), the opposite, it makes easier to review small PRs, although this PR is still in a reasonable size (EDIT: or even small size).

OK, that makes sense, thanks for explaining 😄 Unfortunately I have lots of work to do this week but I hope I will be able to sort out my mess next week 😄 Thanks again! ❤️

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

github-actions · 2023-05-01T22:05:15Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

borpin · 2023-05-02T09:01:30Z

Be good if this could be progressed.

gjohansson-ST · 2023-06-20T21:21:39Z

homeassistant/components/emoncms/sensor.py

-    def native_value(self):
-        """Return the state of the device."""
-        return self._state
+    def native_value(self) -> str | float | None:


You can remove this property and use attr_native_value directly in the update instead.

gjohansson-ST · 2023-06-20T21:25:40Z

homeassistant/components/emoncms/sensor.py

        """Initialize the data object."""
-        self._apikey = apikey
+        self._sess = requests.Session()


This would not be allowed. See https://developers.home-assistant.io/docs/development_checklist

home-assistant · 2023-06-20T21:25:51Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

github-actions · 2023-09-18T22:05:08Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

borpin · 2023-09-19T20:28:40Z

@bwduncan - do you think you will get a chance to look at this again?

bwduncan · 2023-09-19T20:32:03Z

I can try. If the requirement to tidy this up a bit is to create an entire package on pypi just to wrap requests.Session then I don't think I have the appetite for that...

frenck · 2023-11-29T22:07:23Z

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

bwduncan added 2 commits January 27, 2023 20:06

Remove unused hass arguments/attributes

1a13f4e

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Remove unused interval argument

e9437df

Unless I'm missing something obvious, it is never used in the EmonCmsData class, which is instead decorated by @Throttle(MIN_TIME_BETWEEN_UPDATES) If this is supposed to work then it is a bug. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

home-assistant bot added bugfix cla-signed code-quality integration: emoncms new-feature Quality Scale: No score labels Jan 28, 2023

bwduncan added 8 commits January 28, 2023 14:33

Use requests.Session

028a8c3

emoncms.org allows Keep-Alive so use a requests.Session object which allows urllib3 connection pooling. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Convert to _attr_* and fix unique_id

e6c1cd6

Use _attr_name, _attr_native_unit_of_measurement and _attr_native_value. Convert the _identifier into a _attr_unique_id Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Simplify get_id

f22705d

It always takes the same elements of a dict, so just pass the dict. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Factor out native_value

6c2a0ad

native_value is set twice, so deduplicate the code Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Add typing

db33631

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Set state_class to measurement

58968b1

Set state_class even if the unit of measurement is not recognised, since the value will certainly be numeric or null. Emoncms can only return numeric values or null. Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

Add signal strength device class

cc25390

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

bwduncan force-pushed the emoncms-cleanup branch from c7df0af to cc25390 Compare January 28, 2023 14:33

thecode reviewed Jan 28, 2023

View reviewed changes

homeassistant/components/emoncms/sensor.py Show resolved Hide resolved

bwduncan added 3 commits January 30, 2023 10:51

Pylint and mypy fixes

1f6e918

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

black

8bb045f

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

mypy

e6e1276

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

epenet reviewed Jan 31, 2023

View reviewed changes

URL and API key are compulsory

99bee96

Signed-off-by: Bruce Duncan <bwduncan@gmail.com>

github-actions bot added the stale label May 1, 2023

github-actions bot removed the stale label May 2, 2023

gjohansson-ST requested changes Jun 20, 2023

View reviewed changes

home-assistant bot marked this pull request as draft June 20, 2023 21:25

waako mentioned this pull request Aug 20, 2023

Add Config Flow to EmonCMS #94593

Closed

20 tasks

github-actions bot added the stale label Sep 18, 2023

github-actions bot removed the stale label Sep 19, 2023

frenck closed this Nov 29, 2023

github-actions bot locked and limited conversation to collaborators Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emoncms cleanup #86840

Emoncms cleanup #86840

bwduncan commented Jan 28, 2023

home-assistant bot commented Jan 28, 2023

borpin commented Jan 28, 2023

frenck commented Jan 30, 2023

bwduncan commented Jan 30, 2023

epenet Jan 31, 2023

bwduncan Jan 31, 2023

epenet Jan 31, 2023

bwduncan Jan 31, 2023

epenet Jan 31, 2023

bwduncan Jan 31, 2023

thecode Jan 31, 2023 •

edited

Loading

bwduncan Jan 31, 2023

github-actions bot commented May 1, 2023

borpin commented May 2, 2023

gjohansson-ST Jun 20, 2023

gjohansson-ST Jun 20, 2023

home-assistant bot commented Jun 20, 2023

github-actions bot commented Sep 18, 2023

borpin commented Sep 19, 2023

bwduncan commented Sep 19, 2023

frenck commented Nov 29, 2023

Emoncms cleanup #86840

Emoncms cleanup #86840

Conversation

bwduncan commented Jan 28, 2023

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jan 28, 2023

borpin commented Jan 28, 2023

frenck commented Jan 30, 2023

bwduncan commented Jan 30, 2023

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

thecode Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 1, 2023

borpin commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

home-assistant bot commented Jun 20, 2023

github-actions bot commented Sep 18, 2023

borpin commented Sep 19, 2023

bwduncan commented Sep 19, 2023

frenck commented Nov 29, 2023

thecode Jan 31, 2023 •

edited

Loading