Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Elexa Guardian water valve controllers #34627

Merged
merged 14 commits into from May 26, 2020

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Apr 24, 2020

Proposed change

This PR adds support for Elexa Guardian water valve controllers. Per ADR-0010, it can only be configured via the UI (either manually or via zeroconf).

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

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

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:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

Dev automation moved this from Needs review to Reviewer approved May 25, 2020
Copy link
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.

Nice job.

Some minor nits above.

 tests/components/guardian/test_config_flow.py ✓✓✓✓✓✓                                                                                            100% ██████████

----------- coverage: platform linux, python 3.8.1-final-0 -----------
Name                                               Stmts   Miss  Cover   Missing
--------------------------------------------------------------------------------
homeassistant/components/guardian/config_flow.py      40      0   100%
homeassistant/components/guardian/const.py            17      0   100%
--------------------------------------------------------------------------------
TOTAL                                                 57      0   100%


Results (1.03s):
       6 passed

homeassistant/components/guardian/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/guardian/__init__.py Outdated Show resolved Hide resolved
@bdraco bdraco merged commit 369745c into home-assistant:dev May 26, 2020
Dev automation moved this from Reviewer approved to Done May 26, 2020
return unload_ok


class Guardian:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not using our data update coordinator helper? It looks like this class is reimplementing many things from our coordinator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MartinHjelmare It makes use of multiple API calls and chooses which API calls to use based on how many entities have registered interest in them. I didn't want to create multiple data coordinators when I could create one object that handles everything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would have been a lot easier to use one coordinator per API call. This is complicated logic that we don't want to duplicate in each integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure I agree it’s easier. Let’s take the convo offline.

@bachya bachya deleted the guardian branch May 27, 2020 20:00
kennedyshead pushed a commit to kennedyshead/home-assistant that referenced this pull request May 28, 2020
…t#34627)

* Add support for Elexa Guardian water valve controllers

* Zeroconf + cleanup

* Sensors and services

* API registration

* Service bug fixes

* Fix bug in cleanup

* Tests and coverage

* Fix incorrect service description

* Bump aioguardian

* Bump aioguardian to 0.2.2

* Bump aioguardian to 0.2.3

* Proper entity inheritance

* Give device a proper name

* Code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants