Support Yardian YC models#172347
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Yardian integration to use the client factory pattern and adds global services, while adjusting tests and snapshots to reflect new entity/service behavior.
Changes:
- Refactor config flow and setup to use
AsyncYardianClient.createand store extra device info in config entries. - Add
start_irrigation/stop_all_irrigationservices (plus icon/translation/service schema updates). - Update Yardian tests and snapshots to align with changed entity IDs/naming and service behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/yardian/test_switch.py | Makes switch tests dynamic by selecting entities from the state machine and relaxes client call assertions. |
| tests/components/yardian/test_sensor.py | Uses unique_id lookups in entity registry for diagnostic sensor assertions. |
| tests/components/yardian/test_config_flow.py | Updates config flow tests to patch AsyncYardianClient.create and changes expected entry title. |
| tests/components/yardian/snapshots/test_sensor.ambr | Updates snapshots to match new/changed sensor entity IDs and attributes. |
| tests/components/yardian/snapshots/test_binary_sensor.ambr | Updates snapshots to match new/changed binary sensor entity IDs and attributes. |
| tests/components/yardian/conftest.py | Refactors Yardian client mocks to use create() and AsyncMock for async methods. |
| homeassistant/components/yardian/translations/en.json | Adds English translations for config flow and service UI strings. |
| homeassistant/components/yardian/strings.json | Updates service strings and removes entity translation definitions. |
| homeassistant/components/yardian/services.yaml | Tweaks duration unit formatting and adds stop_all_irrigation service schema. |
| homeassistant/components/yardian/icons.json | Updates service icons and adds icon for stop_all_irrigation. |
| homeassistant/components/yardian/const.py | Updates product name, default duration, and introduces a PLATFORMS constant. |
| homeassistant/components/yardian/config_flow.py | Refactors user step to use AsyncYardianClient.create, set unique_id, and include device info in entry data. |
| homeassistant/components/yardian/init.py | Adds global services registration, switches setup to AsyncYardianClient.create, and stores coordinators in hass.data. |
|
|
||
| for entity_id in call.data[ATTR_ENTITY_ID]: |
| hass.services.async_register(DOMAIN, "stop_all_irrigation", handle_stop_all) | ||
| hass.services.async_register(DOMAIN, "start_irrigation", handle_start_irrigation) |
| entity_reg = er.async_get(hass) | ||
| if not (yardian_data := hass.data.get(DOMAIN)): | ||
| return | ||
| duration = call.data.get("duration", 6) |
|
|
||
| await self.async_set_unique_id(str(yid)) | ||
| self._abort_if_unique_id_configured() | ||
|
|
||
| return self.async_create_entry( | ||
| title=f"{PRODUCT_NAME} ({user_input[CONF_HOST]})", | ||
| data={**user_input, **info}, | ||
| ) |
| DOMAIN, context={"source": config_entries.SOURCE_USER} | ||
| ) | ||
|
|
||
| mock_factory_error = _get_mock_client_factory(side_effect=NotAuthorizedException) |
| # Grab the first switch directly from the state machine | ||
| switches = hass.states.async_entity_ids(SWITCH_DOMAIN) | ||
| assert switches, "No switch entities found in the state machine!" | ||
| switches.sort() | ||
| entity_id = switches[0] |
| # Assert it was called, without enforcing strict argument matching | ||
| mock_yardian_client.start_irrigation.assert_called_once() |
| from .const import DOMAIN | ||
| from .coordinator import YardianConfigEntry, YardianUpdateCoordinator | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| PLATFORMS: list[Platform] = [ | ||
| Platform.BINARY_SENSOR, | ||
| Platform.SENSOR, | ||
| Platform.SWITCH, | ||
| ] | ||
|
|
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
I have no clue what you are trying to achieve with this PR. The PR description and title make it sound like you add a new integration, but the integration already exists. Instead a lot of entities seem to break now.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
It's not a new integration. I would like to add new features (services). |
|
I need to do a git push to include my latest changes. |
|
I saw many comments from Copilot, so I was trying to fix them. |
|
I mean, even the first commit was not really doing what it's doing now, so I think you need to zoom out and look what you want to do. As in, we want PRs to be small and easy to review. So look at a single bug you want to fix or feature to add, and just change that and open separate PRs for other things |
|
Got it. I will clean up and submit again. |
aeon-matrix
force-pushed
the
yardian-feature-c1
branch
from
f791e14 to
e40cb54
Compare
|
My question is still the same, what are you intending this PR to do? Because it's again touching a ton of code without doing anything sensible |
|
The goal of this PR is to add irrigation control services (start/stop/stop_all) to work with the updated pyyardian 1.3.3. This enables support for both our original and new firmware versions. I’ve also added tests to achieve 100% coverage and ensure stability across all hardware. |
|
Our new "standalone" firmware allows for 100% local control without internet access, aligning with HA’s core goals. |
|
Sure, but this PR is doing way more than just adding a service, hence I said last time to take a step back and try to make it so that the PR does one thing at a time |
|
I separated the whole change into a few "git commit". Do you want me to turn each commit into a different PR? |
|
I think a lot of commits here are unneeded, but yes, smaller. Want to clean up code? New PR. Want to bump the dependency? New PR. Want to add actions? New PR. Want to refactor existing actions? New PR. That way it's easy to review and speeds up the process |
|
Got it. |
aeon-matrix
force-pushed
the
yardian-feature-c1
branch
from
323d13e to
990d844
Compare
|
Thanks for the suggestion! I will fix the "pytest" for this PR. |
aeon-matrix
force-pushed
the
yardian-feature-c1
branch
from
990d844 to
e833287
Compare
|
Okay, but the PR still changes too many unrelated files. And like said I am questioning the way we implement this |
|
When I added a service, I need to update other related files like the icons.json, __initi__py., etc. Do I need to worry about those test results? I modified many things to make these formatting and testing scripts happy. |
|
Let me try again to make this PR even smaller. I am taking over someone else's work, and I need more time to get the whole picture rather than focusing on passing the bots' tests. |
aeon-matrix
force-pushed
the
yardian-feature-c1
branch
from
e833287 to
6f5c8c7
Compare
aeon-matrix
changed the title
|
I just fixed existing testing issues in this PR. |
|
Yea no worries, like I'm happy to make sure you know your way around the codebase. As far as I know the snapshots are not broken. Home Assistant sources it's translations from I think a good first PR would be to split each zone into its own device. After that you can create a button for each device (zone) which stops irrigation. |
aeon-matrix
changed the title
|
I've fixed some bugs in HA to work with the new pyyardian 1.3.3. I think this should be the first step. |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Okay, now we are getting somewhere. I want to push back a single one last time, let's split them into 2. Changing the way we create the client is a valid way of doing things, but it's a new feature. The entity name change seems to be a bugfix as it has been incorrect. So if you create a single PR with the bugfix, then we can take it into the beta and release that earlier as that will improve a faulty experience.
|
Got it! Can I create a new PR for the bug fix (step 1) and use this PR for "new feature" (step 2)? |
|
Yep! It's fine to have multiple PRs open at the same time. That's also a good way to work in parallel |
|
Great. Hope I get it right this time! |
joostlek
changed the title
Breaking change
Proposed change
Support Yardian standalone firmware with pyyardian 1.3.3.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: