More ruff and warning cleanup - #356
Conversation
Gave the warning `Argument of type "Iterable[EntityDescription]" cannot be assigned to parameter "descriptions" of type "Iterable[ZaptecEntityDescription]" in function "create_entities_from_descriptions"`
Brings the remaining errors down to below 50
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
Great work!. I have a few comments it would be good to discuss
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class ZapBinarySensorEntityDescription(BinarySensorEntityDescription): | ||
| class ZapBinarySensorEntityDescription(ZaptecEntityDescription, BinarySensorEntityDescription): |
There was a problem hiding this comment.
If I'm not mistaken, doesn't this result in a diamond inheritance? Does this have any practical effects in this usage? I've been taught to avoid them like the plague.
There was a problem hiding this comment.
I'll be honest, I'd never heard of diamond inheritance before. Having said that, is this really any different from
class ZaptecBinarySensor(ZaptecBaseEntity, BinarySensorEntity):
a couple of lines above this? Both inherit from the Entity class (if you dig down a bit). Also, these particular classes only add (non-overlapping) variables, so there's no ambiguity.
This was done to avoid the warning Argument of type "Iterable[EntityDescription]" cannot be assigned to parameter "descriptions" of type "Iterable[ZaptecEntityDescription]" in function "create_entities_from_descriptions". Unsure about the practical effects.
|
@steinmn Do we want this in place before the pending 0.8.6 release? |
I don't think so. It's a lot of small changes which (hopefully) only impacts code quality stuff, so probably better to just get a stable version out and then have some more time to discuss all the points above. |
|
This is a long standing PR and I'm inclined to just get it in there and see what happens. I do not have any detailed objections to it. Any objections to merging this into master at this point? We can probably observe the fallout as we go, right? |
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
LGTM - this takes us closer to a better linted codebase, while not 100% perfect, its good enough to move us forward.
Agreed 👍 |
2 participants
Bringing the number of ruff errors down below 50.
Part of #258