-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
Add evaporative humidifier for switchbot integration #146235
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 evaporative humidifier for switchbot integration #146235
Conversation
Hey there @Danielhiversen, @RenierM26, @murtas, @Eloston, @dsypniewski, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
c155a2e
to
1731b68
Compare
64e13b0
to
444c7ae
Compare
@joostlek could you help to review this pr? |
async def async_set_humidity(self, humidity: int) -> None: | ||
"""Set new target humidity.""" | ||
_LOGGER.debug("Setting target humidity to: %s %s", humidity, self._address) | ||
self._last_run_success = bool(await self._device.set_target_humidity(humidity)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more a general comment, but currently we set _last_run_success
and I think instead we should raise a translated exception if the integration did not do what we expect it to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if it fails, it doesn't set the last update success anymore right? I think this is something we can deprecate and remove from all switchbot entities
"water_level": SensorEntityDescription( | ||
key="water_level", | ||
translation_key="water_level", | ||
icon="mdi:water-check", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use icon translations for this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
444c7ae
to
f86c4e9
Compare
@property | ||
def icon(self) -> str | None: | ||
"""Return the icon based on the sensor's state.""" | ||
|
||
if self.entity_description.key != "water_level": | ||
return super().icon | ||
icons = { | ||
"empty": "mdi:water-off", | ||
"low": "mdi:water-outline", | ||
"medium": "mdi:water", | ||
"high": "mdi:water-check", | ||
} | ||
return icons.get(str(self.native_value), "mdi:water-alert") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use icon translations instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed this
"state_attributes": { | ||
"last_run_success": { | ||
"state": { | ||
"true": "[%key:component::binary_sensor::entity_component::problem::state::off%]", | ||
"false": "[%key:component::binary_sensor::entity_component::problem::state::on%]" | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these arent used anymore right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these arent used anymore right
i have removed from this pr, but others not, I will remove all of these in a Independent PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we need to deprecate them, so I would suggest to add a comment to the places where we set it, and then just set the variable like we did before, write state, and then raise. That PR would be marked as deprecation and we should explain in the breaking changes section what is changing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the newly added products, we deprecate the last_run_success attribute. For the already supported products, I will remove this attribute and make a breaking statement. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newly added products won't get it at all, while we need to deprecate it for existing devices and announce it first and then remove in 6 months
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we should still remove it here right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we should still remove it here right
I have removed it now and i have create a PR #147473 to remove for other products
"water_level": { | ||
"name": "Water level", | ||
"state": { | ||
"empty": "Empty", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change
Proposed change
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: