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 Valve integration #102184

Merged
merged 95 commits into from Dec 18, 2023
Merged

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Oct 17, 2023

Proposed change

This adds the valve integration discussed in home-assistant/architecture#975

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

Most of the code is taken from the cover integration but simplified since valves can't tilt.

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][dev-checklist]
  • I have followed the [perfect PR recommendations][perfect-pr]
  • 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][docs-repository]

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

  • The [manifest file][manifest-docs] 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:

@cibernox
Copy link
Contributor Author

This is a WIP but I wanted put it in the wild now.

There are a couple outstanding errors I'm not sure how to solve and prevents me from even making this commit without --no-verify, if anyone has any directions I'd appreciate it.

* [ERROR] [MANIFEST] Domain is missing an IoT Class

What iot_class should an integration that is more like an abstract contract for other integrations to build on top of have?

* [ERROR] [SERVICES] Invalid services.yaml: Unknown supported feature 'valve.ValveEntityFeature.CLOSE' @ data['close_valve']['target']['entity'][0]['supported_features'][0]. Got None

I have a couple errors similar to these but I can't figure out why I get them, I see no difference with how other services manifest and integration handle supported features.

homeassistant/components/valve/device_action.py Outdated Show resolved Hide resolved
homeassistant/components/valve/group.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

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.

@home-assistant home-assistant bot marked this pull request as draft October 18, 2023 06:03
@MartinHjelmare
Copy link
Member

There are a couple outstanding errors I'm not sure how to solve and prevents me from even making this commit without --no-verify, if anyone has any directions I'd appreciate it.

* [ERROR] [MANIFEST] Domain is missing an IoT Class

What iot_class should an integration that is more like an abstract contract for other integrations to build on top of have?

Add the integration domain name to the Platform enum. Then we don't need to set an IoT class for the integration.

https://github.com/home-assistant/core/blob/348e95d026f80fad1ba8b97510d5fdcbda5ab551/homeassistant/const.py#L22

@MartinHjelmare
Copy link
Member

* [ERROR] [SERVICES] Invalid services.yaml: Unknown supported feature 'valve.ValveEntityFeature.CLOSE' @ data['close_valve']['target']['entity'][0]['supported_features'][0]. Got None

Update the selectors helper:

@cache
def _entity_features() -> dict[str, type[IntFlag]]:
"""Return a cached lookup of entity feature enums."""
# pylint: disable=import-outside-toplevel
from homeassistant.components.alarm_control_panel import (
AlarmControlPanelEntityFeature,
)
from homeassistant.components.calendar import CalendarEntityFeature
from homeassistant.components.camera import CameraEntityFeature
from homeassistant.components.climate import ClimateEntityFeature
from homeassistant.components.cover import CoverEntityFeature
from homeassistant.components.fan import FanEntityFeature
from homeassistant.components.humidifier import HumidifierEntityFeature
from homeassistant.components.lawn_mower import LawnMowerEntityFeature
from homeassistant.components.light import LightEntityFeature
from homeassistant.components.lock import LockEntityFeature
from homeassistant.components.media_player import MediaPlayerEntityFeature
from homeassistant.components.remote import RemoteEntityFeature
from homeassistant.components.siren import SirenEntityFeature
from homeassistant.components.update import UpdateEntityFeature
from homeassistant.components.vacuum import VacuumEntityFeature
from homeassistant.components.water_heater import WaterHeaterEntityFeature
from homeassistant.components.weather import WeatherEntityFeature
return {
"AlarmControlPanelEntityFeature": AlarmControlPanelEntityFeature,
"CalendarEntityFeature": CalendarEntityFeature,
"CameraEntityFeature": CameraEntityFeature,
"ClimateEntityFeature": ClimateEntityFeature,
"CoverEntityFeature": CoverEntityFeature,
"FanEntityFeature": FanEntityFeature,
"HumidifierEntityFeature": HumidifierEntityFeature,
"LawnMowerEntityFeature": LawnMowerEntityFeature,
"LightEntityFeature": LightEntityFeature,
"LockEntityFeature": LockEntityFeature,
"MediaPlayerEntityFeature": MediaPlayerEntityFeature,
"RemoteEntityFeature": RemoteEntityFeature,
"SirenEntityFeature": SirenEntityFeature,
"UpdateEntityFeature": UpdateEntityFeature,
"VacuumEntityFeature": VacuumEntityFeature,
"WaterHeaterEntityFeature": WaterHeaterEntityFeature,
"WeatherEntityFeature": WeatherEntityFeature,
}

@cibernox
Copy link
Contributor Author

@MartinHjelmare thanks, I can commit normally now! I'll try to have this ready tonight, pending documentation.

This adds the valve integration discussed in home-assistant/architecture#975
Most of the code is taken from the cover integration but simplified since valves
can't tilt.

There are a couple outstanding errors I'm not sure how to solve and prevents
me from even making this commit without `--no-verify`.
@jbouwh
Copy link
Contributor

jbouwh commented Oct 25, 2023

@cibernox let me know if I can help

@cibernox
Copy link
Contributor Author

@jbouwh I just pushed some changes with the feedback (mostly removing stuff so the functionality is minimal. Those can be added at a later date).

If CI thinks this is good, I'll start writing docs. I may mention you to review and proof-read them, english is not my first language after all.

@jbouwh
Copy link
Contributor

jbouwh commented Oct 25, 2023

@jbouwh I just pushed some changes with the feedback (mostly removing stuff so the functionality is minimal. Those can be added at a later date).

If CI thinks this is good, I'll start writing docs. I may mention you to review and proof-read them, english is not my first language after all.

English isn't my first language either.

As log as homeassistant/components/poolstation/sensor.py is not removed, CI tests will keep failing.

@cibernox
Copy link
Contributor Author

CI seems happy, now the docs.

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Very nice, thanks @cibernox 👍

@cibernox
Copy link
Contributor Author

@emontnemery thank you for the last push!

tests/components/valve/test_init.py Outdated Show resolved Hide resolved
tests/components/valve/test_init.py Outdated Show resolved Hide resolved
@MartinHjelmare
Copy link
Member

We have the frontend part in progress. When the frontend PR is approved and linked in this PR description, we can merge here.

@piitaya
Copy link
Member

piitaya commented Dec 12, 2023

Front-end PR : home-assistant/frontend#19024

@jbouwh jbouwh mentioned this pull request Dec 14, 2023
20 tasks
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

We're changing our base entity descriptions to be keyword only and frozen. Since there are no existing platforms for the valve integration we can set it here without backwards compatibility layer.

homeassistant/components/valve/__init__.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft December 16, 2023 11:56
@piitaya
Copy link
Member

piitaya commented Dec 18, 2023

Front-end is merged

@frenck
Copy link
Member

frenck commented Dec 18, 2023

Thanks, @piitaya 👍

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@emontnemery emontnemery marked this pull request as ready for review December 18, 2023 14:47
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

tests/components/valve/test_init.py Outdated Show resolved Hide resolved
tests/components/valve/test_init.py Outdated Show resolved Hide resolved
@emontnemery emontnemery reopened this Dec 18, 2023
@MartinHjelmare MartinHjelmare merged commit 5175737 into home-assistant:dev Dec 18, 2023
53 checks passed
@cibernox
Copy link
Contributor Author

🎉 🎉 🎉

Bre77 pushed a commit to Bre77/core that referenced this pull request Dec 18, 2023
* Add Valve integration.

This adds the valve integration discussed in home-assistant/architecture#975
Most of the code is taken from the cover integration but simplified since valves
can't tilt.

There are a couple outstanding errors I'm not sure how to solve and prevents
me from even making this commit without `--no-verify`.

* Apply PR feedback

* Apply more feedback: Intruduce the bare minimum

* Remove file commited by mistake

* Hopefully this fixes tests

* Match cover's typing and mypy settings

* Change some configuration files

* Fix test

* Increase code coverage a little

* Code coverate inproved to 91%

* 95% code coverage

* Coverate up to 97%

* Coverage 98%

* Apply PR feedback

* Even more feedback

* Add line I shouldn't have removed

* Derive closed/open state from current position

* Hopefully last feedback

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Remove unnecesary translation

* Remove unused method arguments

* Complete code coverage

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Simplify tests

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Apply last feedback

* Update tests/components/valve/test_init.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Update tests/components/valve/test_init.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Update tests/testing_config/custom_components/test/valve.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* More feedback

* Apply suggestion

* And more feedback

* Apply feedback

* Remove commented code

* Reverse logic to unindent

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Implement stop valve for Mock valve

* Fix tests now that I've implemented stop_valve

* Assert it's neither opening nor closing

* Use current position instead

* Avoid scheduling executor when opening or closing

* Fix incorrect bitwise operation

* Simplify toggle

* Remove uneeded partial functions

* Make is_last_toggle_direction_open private

* Remove valve from test custom integration

* Improve test coverage

* Address review comments

* Address review comments

* Address review comments

* Update homeassistant/components/valve/__init__.py

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Update tests

---------

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Erik <erik@montnemery.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants