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.
| def name(self) -> str: | ||
| """Return the name of the object.""" | ||
| return self._attrs["name"] | ||
| return str(self._attrs["name"]) |
| class TLogExc(Protocol): | ||
| """Protocol for logging exceptions.""" | ||
|
|
||
| def __call__(self, exc: Exception) -> Exception: ... |
There was a problem hiding this comment.
We should except this ruff error (D102) for this line. The protocol using ellipsis is correct.
|
|
||
| @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.
| 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?
| def iter_objects( | ||
| service_call: ServiceCall, mustbe: type[T] | ||
| ) -> Generator[tuple[ZaptecUpdateCoordinator, T], None, None]: | ||
| ) -> Generator[tuple[ZaptecUpdateCoordinator, T]]: |
There was a problem hiding this comment.
Because it's a fact that this code will always be run on py3.13+, yes?
There was a problem hiding this comment.
Pretty sure HA dropped python3.12 before the 2025.7 version we list as minimum, but I'll double check once I have a bit more time to devote.
|
|
||
| data = await request(url := f"chargers/{charger_id}/state") | ||
| redact.redact_statelist(data, ctx="state") | ||
| data = redact.redact_statelist(data, ctx="state") |
There was a problem hiding this comment.
This is a good catch. Because redact_statelist is currently implicitly modifying data. But later implementations might not, so doing this is good practice.
|
|
||
| # 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?
| 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.
| if "Authorization" in headers: | ||
| headers["Authorization"] = "<Removed for security>" | ||
| yield f" headers {dict((k, v) for k, v in headers.items())}" | ||
| yield f" headers {dict(headers.items())}" |
There was a problem hiding this comment.
Which is the same as headers.copy() isn't it? Which in turn is the same as just doing {headers}?
I don't remember why this construct is like this. I suspect there used to be a if to that dict comprehension that has been removed.
|
@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