Add and fix some Huawei LTE type hints #41196
Conversation
probot-home-assistant
bot
added
Hacktoberfest
code-quality
integration: huawei_lte
labels
|
Hey there @fphammerle, mind taking a look at this pull request as its been labeled with an integration ( |
| @@ -275,7 +275,7 @@ def logout(self) -> None: | |||
| except Exception: # pylint: disable=broad-except | |||
| _LOGGER.warning("Logout error", exc_info=True) | |||
|
|
|||
| def cleanup(self, *_) -> None: | |||
| def cleanup(self, *_: Any) -> None: | |||
There was a problem hiding this comment.
just cosmetics but maybe:
| def cleanup(self, *_: Any) -> None: | |
| def cleanup(self, *_: Tuple) -> None: |
There was a problem hiding this comment.
self.cleanup gets passed to HomeAssistant.async_add_executor_job and EventBus.async_listen_once, and the relevant parameter in the first is Callable[..., T], and in the latter it's just Callable. As we are not affecting what actually gets passed to this function as parameters, I don't think it would be quite appropriate to make the type hints more specific here -- that would be manifesting internal implementation detail of those functions more than they currently expose in their type hints themselves.
(It would be good to make those type hints more specific if possible, but that's something for a different PR.)
| @@ -81,7 +82,7 @@ def async_add_new_entities(hass, router_url, async_add_entities, tracked): | |||
| _LOGGER.debug("%s[%s][%s] not in data", KEY_WLAN_HOST_LIST, "Hosts", "Host") | |||
| return | |||
|
|
|||
| new_entities = [] | |||
| new_entities: List[Entity] = [] | |||
There was a problem hiding this comment.
maybe more specific?
| new_entities: List[Entity] = [] | |
| new_entities: List[HuaweiLteScannerEntity] = [] |
There was a problem hiding this comment.
should still be compatible with async_add_entities as Entity inherits from HuaweiLteScannerEntity, right?
same question for HuaweiLteSensor / async_add_entities below
There was a problem hiding this comment.
List is not covariant, so this would cause mypy to barf:
homeassistant/components/huawei_lte/device_tracker.py:103: error: Argument 1 has incompatible type "List[HuaweiLteScannerEntity]"; expected "List[Entity]"
homeassistant/components/huawei_lte/device_tracker.py:103: note: "List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance
homeassistant/components/huawei_lte/device_tracker.py:103: note: Consider using "Sequence" instead, which is covariant
(Aside, our function parameter type hints should in general be changed towards abstract classes ("interfaces") as appropriate instead of using concrete types like List (concrete return types are fine). That would be the right thing to do in general, and would improve things in this case as a side effect.)
| """Turn switch on.""" | ||
| self._turn(state=True) | ||
|
|
||
| def turn_off(self, **kwargs): | ||
| def turn_off(self, **kwargs: Any) -> None: |
There was a problem hiding this comment.
maybe:
| def turn_off(self, **kwargs: Any) -> None: | |
| def turn_off(self, **kwargs: Dict[str, Any]) -> None: |
same for turn_on
There was a problem hiding this comment.
I don't think that's necessary or idiomatic, and I suppose it would be actually wrong. Keyword arguments always come as str keyed dicts, and although I'm unable to locate a doc reference to back it up, I think **kwargs: Any along with some type checker magic is a shorthand notation for that.
Anyway, turn_on and turn_off are explicitly hinted with **kwargs: Any in ToggleEntity where they originate from, and like in the cleanup case above, we're not affecting in any way what actually gets passed to them as parameters nor using them, so copying the superclass hints is the right thing to do.
There was a problem hiding this comment.
Found the missing doc reference: https://www.python.org/dev/peps/pep-0484/#arbitrary-argument-lists-and-default-argument-values
def foo(*args: str, **kwds: int): ...
In the body of function foo, the type of variable args is deduced as Tuple[str, ...] and the type of variable kwds is Dict[str, int].
There was a problem hiding this comment.
Thanks for all the explanations and docs reference!
Proposed change
Adds a bunch of trivialish type hints to huawei_lte, to get the easily reviewable ones out of the way of slightly bigger related changes.
Type 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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: