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
| def id(self) -> str: | ||
| """Return the id of the object.""" | ||
| return self._attrs["id"] | ||
| return str(self._attrs["id"]) |
There was a problem hiding this comment.
Can self._attrs["id"] ever be anything else than str? While I understand this helps with the type, it will return the string "None" if the object is None. Should it rather raise an exception if the object is non-str? -- I'm not sure what's best here, so I'm open to a discussion.
There was a problem hiding this comment.
Can the object be None? _attrsis a TDict, where the values can be str | int | float | bool, so I don't think None should be an option? And if the key doesn't exist, then the code will raise a KeyError, right?
| def name(self) -> str: | ||
| """Return the name of the object.""" | ||
| return self._attrs["name"] | ||
| return str(self._attrs["name"]) |
|
|
||
| @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.
| return await _get_diagnostics(hass, config_entry) | ||
| except Exception: | ||
| _LOGGER.exception("Error getting diagnostics") | ||
| return {} |
There was a problem hiding this comment.
What's correct behavior here? Is it to return nothing like everything is ok, or would a more correct approach be to escalate the error to HA? What does other integrations do?
There was a problem hiding this comment.
Looking a bit more thoroughly at it, I don't see how we can ever reach this line of code anyway. Pretty much all of _get_diagnostics is try-excepted, so it would have to fail on add_failure I think? Part of me wants to delete the entire try-except here, and the more risk-averse part of me wants to leave it as-is.
|
|
||
| # Remove data fields with excessive data, making it bigger than the | ||
| # HA database appreciates for the size of attributes. | ||
| # FIXME: SupportGroup is sub dict. This is not within the declared type |
There was a problem hiding this comment.
I suggest excluding the TODO or FIXME linting comments. Or do you think they should be there as a constant reminder that something isn't complete?
There was a problem hiding this comment.
Well, all the TODO or FIXME linting comments are about this single line, and I'm not sure what it actually means, what needs to be fixed here? Should the FIXME be replaced with a NOTE instead?
There was a problem hiding this comment.
If it is indeed something that needs to be fixed, I think disabling the FIX002-error is fine, but I kind of agree with the other 3 (requiring an author, an issue link to the issue where it will be fixed, and for the keyword to be TODO).
So for instance, this will not give any ruff errors:
# TODO(@sveinse): #258 SupportGroup is sub dict. This is not within the declared type # noqa: FIX002
| def __getitem__(self, obj_id: str) -> ZaptecBase: | ||
| """Get an object data by id.""" | ||
| return self._map[id] | ||
| return self._map[obj_id] |
There was a problem hiding this comment.
/pedantic: I think the standard argument for __getitem__ is item. Since that we use for all the other non-dunder methods I think this is fine.
There was a problem hiding this comment.
Here it looks like it's key. I think I prefer obj_id to both alternatives.
|
@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. |
2 participants
Bringing the number of ruff errors down below 50.
Part of #258