Fix more simple ruff errors, mostly typing#299
Fix more simple ruff errors, mostly typing#299sveinse merged 2 commits intocustom-components:masterfrom
Conversation
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
Looks ok, with some comments that should be looked at.
Getting a lot of "PLR0915 Too many statements (65 > 50)" and "PLR0912 Too many branches (17 > 12)".
These are disabled in HA core: https://github.com/home-assistant/core/blob/8175f4ba6575b5e8deecde68e88cc95171a6e796/pyproject.toml#L758-L761
custom_components/zaptec/__init__.py
Outdated
| ZAPTEC_POLL_INTERVAL_INFO, | ||
| ) | ||
| from .services import async_setup_services, async_unload_services | ||
| import contextlib |
There was a problem hiding this comment.
Please run reformat imports, so this is ordered where it should be.
|
|
||
| import aiohttp | ||
| from aiolimiter import AsyncLimiter | ||
| from azure.servicebus.exceptions import ServiceBusError |
There was a problem hiding this comment.
Importing the service bus is optional. It's done run-time with a try...except in the function, so importing it here will negate that logic. We can do one of two things: We can either import everything here at the top or this import is put in a if TYPE_CHECKING: block.
There was a problem hiding this comment.
Is there any benefit to not importing everything at the top? Considering the installation is enforced by manifest.json anyway.
There was a problem hiding this comment.
Perhaps we should move all imports to the top. Linters often complain if doing runtime imports.
I think this is a remnant of older code where service bus wasn't always available and created conflicts. Aka to what we see with pydantic. I think I remember azure-servicebus not being even a listed dependency. Users had to install it themselves if they needed it. When the right service bus is always available (which it must be to get it installed), then there is nothing to gain from doing runtime import I think.
There was a problem hiding this comment.
Had a look through now, and the only imports not on top are now in the if __name__ == "__main__": sections of
- api.py
- diagnostics.py
- misc.py
I'm thinking we leave those as-is until we migrate them to proper tests in #262?
There was a problem hiding this comment.
Yes please. I often use the api and diagnostics to extract information without needing to fire up HA. I agree the test parts should be moved to pytest, but I also think maybe we need a small cmd-line tool for such things.
| err_device = f"{lookup[uid]} ({uid})" | ||
| else: | ||
| err_device = f"id {uid}" | ||
| err_device = f"{lookup[uid]} ({uid})" if uid in lookup else f"id {uid}" |
There was a problem hiding this comment.
This is ok, but I know that a lot of py-people frown at this because it has so many terms and becomes much harder to read1. A if B else C. I'm curious, did ruff suggest this?
Footnotes
There was a problem hiding this comment.
Yes it (currently) recommends it with explanation https://docs.astral.sh/ruff/rules/if-else-block-instead-of-if-exp/
I do see that SIM108 is among the ignores in HA (https://github.com/sveinse/ha-core/blob/facf217b995982ff5c504bd59d419a59c31ee068/pyproject.toml#L772C4-L772C10). I'm inclined to agree with the "if it fits in a single line, it's good"-criteria in SIM108, but I'm not fanatical about it.
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
LGTM
What do you think about the "FIrst line of docstring should be in imperative mood." comments from ruff?
|
Don't really have strong meanings on that one. Looking at the ha-pyproject, it seems they use the google convention which according to https://docs.astral.sh/ruff/rules/non-imperative-mood/ disables D401 (even if it's not explicitly stated in ha-pyproject. So I think we can turn it off if you don't like it 🤷 |
2 participants
No description provided.