core icon indicating copy to clipboard operation
core copied to clipboard

Add Rointe integration

Open tggm opened this issue 3 years ago • 20 comments

Proposed change

This integration provides support for electrical radiators manufactured by Rointe. This is an initial implementation that supports some of Rointe's products:

  • D-Series Radiator
  • Towel Rails
  • Thermostat.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [X] 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

The integration uses cloud polling via a REST API owned by Rointe. Config flow requires the user to login to the API and then select which particular installation they want to add to HA.

It's currently available via HACS on https://github.com/tggm/rointe-hacs/

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/25118

Checklist

  • [X] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the development checklist
  • [X] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

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

  • [X] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] 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.
  • [x] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

tggm avatar Dec 01 '22 19:12 tggm

Thank you for the first look at this @bachya. It was indeed in a very rough state. I cleaned some of the issue and will ping you when I manage to clear the rest.

This latest commit contains a reauth flow I've been working. It also contains a bunch of debug statements which I added to help another user setup this integration (it's currently available via HACS so I have a couple of people beta testing this).

tggm avatar Dec 06 '22 23:12 tggm

This latest commit contains a reauth flow I've been working. It also contains a bunch of debug statements which I added to help another user setup this integration (it's currently available via HACS so I have a couple of people beta testing this).

Per my previous comment, I would recommend that you do a reauth flow in a separate PR—this PR already has plenty in it.

bachya avatar Dec 06 '22 23:12 bachya

Per my previous comment, I would recommend that you do a reauth flow in a separate PR—this PR already has plenty in it.

Will do.

tggm avatar Dec 06 '22 23:12 tggm

@bachya I cleared up the issues. I changed async_unload_entry a bit and during testing I noticed that when removing the integration and then adding it again the entities do not show up (shown as unavailable). They do however appear after rebooting HASS.

No errors, everything appears OK in logs, both removing and adding again. Maybe async_unload_entry is still not correct

tggm avatar Dec 07 '22 19:12 tggm

@bachya I fixed the problem with unloading the integration. It was a stupid error where I was using a class variable to store my entities.

I also updated the code to use async_forward_entry_setups() according to this post

Finally I removed some useless code from async_unload_entry that I'd copied from another integration.

tggm avatar Dec 07 '22 22:12 tggm

Hi @bachya,

Are there any other tasks you want me to work to help with the review?

tggm avatar Dec 15 '22 11:12 tggm

What are the test coverage targets for files other than config_flow.py ?

tggm avatar Dec 18 '22 17:12 tggm

What are the test coverage targets for files other than config_flow.py ?

Depends on your objective. Config flow tests are required; (almost) everything else is discretionary.

More info: https://developers.home-assistant.io/docs/integration_quality_scale_index?_highlight=coverage#gold-

bachya avatar Dec 18 '22 18:12 bachya

What are the test coverage targets for files other than config_flow.py ?

Depends on your objective. Config flow tests are required; (almost) everything else is discretionary.

More info: https://developers.home-assistant.io/docs/integration_quality_scale_index?_highlight=coverage#gold-

Thanks. Right now the objective is getting the integration merged (no-score) and then aim for Silver and up.

I'll keep adding tests in the meantime (assuming that adding new test files is OK during the review process).

I'll remove entries from .coveragerc as soon as individual files reach 100%

tggm avatar Dec 18 '22 20:12 tggm

@bachya Added a couple more tests and fixed minor issues found during tests

tggm avatar Dec 20 '22 19:12 tggm

Hi @bachya !

Do you have any next steps for me?

tggm avatar Jan 02 '23 13:01 tggm

I've reviewed what I can and think this is shaping up nicely, but given its size, I will ask for additional review. A reminder that with new integrations, you'll generally have a faster review if the initial contribution contains only one platform (e.g., sensor).

bachya avatar Jan 05 '23 19:01 bachya

I've reviewed what I can and think this is shaping up nicely, but given its size, I will ask for additional review. A reminder that with new integrations, you'll generally have a faster review if the initial contribution contains only one platform (e.g., sensor).

Thank you for the review @bachya.

I understand that it will take time.

tggm avatar Jan 05 '23 20:01 tggm

Hi @bachya,

How long do you think it will take for the second opinion review? It has been 2 months :cry:

tggm avatar Mar 07 '23 18:03 tggm

I don't know, unfortunately. I'm currently busy at a new job, so I'm not as focused in HASS for the time being.

I recommend you go to Discord, go to the #devs_core channel, and see if anyone would be willing to help you push this across the finish line.

bachya avatar Mar 07 '23 19:03 bachya

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

Learn more about our pull request process.

home-assistant[bot] avatar Jun 20 '23 21:06 home-assistant[bot]

Please strip it down to an implementation of a single platform https://developers.home-assistant.io/docs/review-process Also it has conflicts that needs to be addressed.

I'm sorry, after all the effort and the time this took back and forth in reviews with other people you're are seriously asking me to strip the code down?!

tggm avatar Jun 20 '23 22:06 tggm

Yes, please remove all but one platform. Downsizing a PR is the fastest way to get it merged. And now you even have a reviewer that has requested changes, which normally means they will merge the PR when all the requested changes are complete.

Thanks for your understanding!

MartinHjelmare avatar Jun 22 '23 11:06 MartinHjelmare

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.

github-actions[bot] avatar Sep 20 '23 13:09 github-actions[bot]

Yes, please remove all but one platform. Downsizing a PR is the fastest way to get it merged. And now you even have a reviewer that has requested changes, which normally means they will merge the PR when all the requested changes are complete.

Thanks for your understanding!

Removed the other platforms (update and sensor).

tggm avatar Sep 25 '23 20:09 tggm

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. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar Apr 04 '24 16:04 github-actions[bot]