libgpiod
libgpiod copied to clipboard
bindings: python: consider loosening the type requirements for public interfaces
Creating this issue to track the idea somewhere outside of my head.
Currently, the public API exposed by gpiod is relatively restrictive on what types are required to be used for method calls.
As an example in LineRequest
:
def reconfigure_lines(
self,
config: dict[
Union[tuple[Union[int, str], ...], int, str], Optional[LineSettings]
],
) -> None:
The tuple
argument type is overly strict since the function itself only requires that the type in the dictionary implement __iter__
so this could be retyped as Iterable[Union[int, str]]
Similarly, dict
is a bit too strict when we're just requiring a Mapping
style class.
This typing is a bit trickier, however, as Mapping
doesn't play well with the interface we've defined. As we don't actually modify the mapping, the current typing of the generic Mapping
is a bit restrictive. This delves into invariant/covariant/contravariant semantics which I'm not 100% comfortable with but there have been discussions about having a covariant Mapping
type:
https://github.com/python/typing/issues/445
https://github.com/python/typing_extensions/issues/5
https://github.com/hauntsaninja/useful_types/issues/27
My "bad" idea was defining our own protocol in _internal
that we use for typing these:
_KT_co = TypeVar("_KT_co", covariant=True)
_VT_co = TypeVar("_VT_co", covariant=True)
class _CovariantMapping(Protocol[_KT_co, _VT_co]):
def keys(self) -> KeysView[_KT_co]: ...
def items(self) -> ItemsView[_KT_co, _VT_co]: ...
We only require that there be a keys()
and items()
method implemented on the dict
arguments.
Otherwise, maybe instead of the convoluted "dict of tuple of int or str, or int, or str" typing we currently have, it gets reworked into a new class with different semantics.
Some notes on that:
-
Chip.request_lines
currently checks for duplicates, howeverLineRequest.reconfigure_lines
does not, so if an offset is in the argument multiple times, the last value wins (based on iteration order). - It may make sense to allow inserting int/str/Iterable[int|str] into this object, but it should consume the iterable and insert the value into an internal
offset: dict[int]
ornamed: dict[str]
. Splitting these out allows checking for overlap from the resolved line names to the offsets. - Whether insertion throws an error if the offset or line name already exist should probably be configurable but default to True
- when reconfiguring lines, there should probably be an optional flag to raise an error on duplicate offsets after names get resolved to offsets.