fix for 2024.4 #131
fix for 2024.4 #131
Conversation
Fischelsberger
force-pushed
the
fix_2024.4
branch
from
051d41a to
0a952f3
Compare
|
@golles, I've implemented your suggestions, but only tested it with 2024.4 this time. |
|
Implemented your changes. Am now getting binary result. Can't tell if it's working though, is 'Off" Wash or Don't Wash? |
|
tbh, i'm 99% sure that it always was binary. off = don't wash |
|
It's working. Thank you. I believe it used to say "wash" and "don't_wash". But knowing which is which is good. Apparently it's gonna snow tomorrow, it's sunny today so I was like "why not wash?" Now I know. Thanks for the fix! |
There was a problem hiding this comment.
@Fischelsberger thanks for addressing the comment. This looks good and I've also tested it locally and no issues!
I'm not the owner of this repo so I don't know what @Limych would require. Hopefully, he can have a look at this and also approve the usage of the workflows in this PR so we can see if CI passes.
|
Yes indeed, it's basically the same issue |
| blocking=True, | ||
| return_response=True, | ||
| ) | ||
| forecast = daily_response[self._weather_entity]["forecast"] |
There was a problem hiding this comment.
Please add a check for the presence of a result in the return value (daily_response). And also add a unit test for your code.
There was a problem hiding this comment.
Thank you for the review and requested changes.
I've just added a new commit, with the changes which are looking good to me.
Also changed some imports, because ruff suggested these changes.
| ) | ||
| if not (isinstance(daily_response, dict)): | ||
| self._attr_is_on = None | ||
| raise HomeAssistantError("daily_response is empty.") |
There was a problem hiding this comment.
This error message is not clear to the user if he sees it in the log. The user does not know what daily_response is.
There was a problem hiding this comment.
fixed it with new commit, by just using the original message
"Can't get forecast data! Are you sure it's the weather provider?"
by setting forecast = None
There was a problem hiding this comment.
The option is suitable, but it will be better if the error message is more unique, so that it is also more clear to developers about where the failure actually occurred.
Sorry @Limych, I don't really understand the whole unit test thing... I've tried multiple ways, but I simply can't understand that (right now)... |
|
With unit tests everything is quite simple. We take the existing code as a basis, but we need to check all new branches according to the conditions. It is necessary to patch self.hass.services.async_call in the test and return two variants of results - normal and error. And check that the code reacts correctly to each option. Tomorrow I'll try to find some time and write some sample code. |
|
Closed because these changes are no longer necessary due to the release of a new version of the component. |
Proposed change
Replacing the old "Forecast" Attribut with a service call of "weather.get_forecasts" so that it's working with 2024.4.
Tested it with Homeassistant 2024.3 and 2024.4, working with booth.
Also fixed an issue wich was within that line.
datetime.utcfromtimestamp(fc_date / 1000)Some Extension provided by Homeassistant devcontainer or python itself requested that change...
Type of change
Additional information
Checklist
black --fast custom_components)If user exposed functionality or configuration variables are added/changed: