Plugwise prepare typing for binary_sensor #93162
Conversation
home-assistant
bot
added
cla-signed
code-quality
integration: plugwise
small-pr
|
Hey there @bouwew, @frenck, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| @@ -35,19 +46,22 @@ class PlugwiseBinarySensorEntityDescription(BinarySensorEntityDescription): | |||
| icon="mdi:hvac", | |||
| icon_off="mdi:hvac-off", | |||
| entity_category=EntityCategory.DIAGNOSTIC, | |||
| value_fn=lambda data: data["binary_sensors"]["compressor_state"], | |||
There was a problem hiding this comment.
You could simplify this quite a bit by passing data["binary_sensors"] directly:
| value_fn=lambda data: data["binary_sensors"]["compressor_state"], | |
| value_fn=lambda data: data["compressor_state"], |
There was a problem hiding this comment.
As per Frencks (in #93155) and you suggestion I rewrote the approach, models.py no already shows the intended parts for sensor and switch as well (hence the pragma, pw_key is used in sensor). I know we should stick to only binary_sensor as per keeping it small, but we would like to know if this is going in the right direction.
I took your suggestion of passing the key and combined it to leave lambda's out as requested in the other draft.
There was a problem hiding this comment.
I really think the code became worse after this part. Like the suggestions made here by epenet have been taken and over-engineered IMHO.
The whole lookup stuff is really nog needed at all. All epenet suggested, was simply passing part of the dictionary, instead of all.
CoMPaTech
changed the title
|
I would strongly suggest reverting e0c02d1, it makes everything unneeded complex and not what was originally commented either. |
|
Setting back to draft, agreed with the less complexity + as per #93155 lambda was also not the correct route. Rethinking solution. |
|
This PR original had a fine solution, epenet give a slight hint to make it a little better (which however, was ignored and next added unneeded complexity) |
|
Yup an that comment remains even now, we, we don't need to use lambdas in this case at all. |
|
I've looked at this again. Using lambda would have: value_fn: Callable[[SmileBinarySensors], bool]
...
value_fn=lambda data: data["compressor_state"],
...
return self.entity_description.value_fn(self.device["binary_sensors"])Using a key (separate from the main key to be able to use literal): value_key: Literal[
"cooling_enabled",
"compressor_state",
"cooling_state",
"dhw_state",
"flame_state",
"heating_state",
"plugwise_notification",
"slave_boiler_state",
]
...
value_key="compressor_state",
...
return self.device["binary_sensors"][self.entity_description.value_key] |
|
We do not have to use |
But then you lose the type hints, and end up having to ignore homeassistant/components/plugwise/binary_sensor.py:167: error: Returning Any from function declared to return "Optional[bool]" [no-any-return]
homeassistant/components/plugwise/binary_sensor.py:167: error: TypedDict key must be a string literal; expected one of ("cooling_enabled", "compressor_state", "cooling_state", "dhw_state", "flame_state", ...) [literal-required] |
|
Right makes sense, with that in mind, I think the current lambda is the cleanest path 👍 |
Proposed change
Change entitydescription approach for
binary_sensorpreparing for upcoming strict typing and module version bump PRs. Decreasing PR size as per #92253 / #93088Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: