Avoid limit_current service call passing None-values and clarify error messages#284
Conversation
|
Seems slightly paranoid to fix it both in api.py and in service.py, but on the other hand |
custom_components/zaptec/services.py
Outdated
| if available_current := service_call.data.get("available_current"): | ||
| limit_args["availableCurrent"] = available_current | ||
| if available_current_phase1 := service_call.data.get("available_current_phase1"): | ||
| limit_args["availableCurrentPhase1"] = available_current_phase1 | ||
| if available_current_phase2 := service_call.data.get("available_current_phase2"): | ||
| limit_args["availableCurrentPhase2"] = available_current_phase2 | ||
| if available_current_phase3 := service_call.data.get("available_current_phase3"): | ||
| limit_args["availableCurrentPhase3"] = available_current_phase3 |
There was a problem hiding this comment.
This won't work if the value is being set to 0. It needs to be explicitly tested for None.
There was a problem hiding this comment.
🤦 tried to be a little bit too elegant, fixed now
custom_components/zaptec/api.py
Outdated
| if not (has_availablecurrent ^ all(has_availablecurrentphases)): | ||
| raise ValueError( | ||
| "Either availableCurrent or all of availableCurrentPhase1, " | ||
| "Either availableCurrent OR all of availableCurrentPhase1, " |
There was a problem hiding this comment.
I'm not overly fond of using capital letters in the middle of an English sentence
There was a problem hiding this comment.
Wanted to add emphasis to make it clearer that it's an exclusive or, but I'm fine either way
custom_components/zaptec/api.py
Outdated
| if has_availablecurrent and any(has_availablecurrentphases): | ||
| raise ValueError( | ||
| "A combination of both availableCurrent and any of " | ||
| "availableCurrentPhase1, availableCurrentPhase2, " | ||
| "availableCurrentPhase3 is not allowed, choose one" | ||
| ) |
There was a problem hiding this comment.
This error is a little confusing to me. Perhaps it's better to test that if any of the three (with any()) have used, and all of them must be present. Error: "All of availableCurrent1, availableCurrent2 and availableCurrent2 must be present".
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
LGTM. Thank you for fixing.
2 participants
Fixes #283