libgpiod icon indicating copy to clipboard operation
libgpiod copied to clipboard

bindings: python: consider loosening the type requirements for public interfaces

Open vfazio opened this issue 5 months ago • 5 comments

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, however LineRequest.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] or named: 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.

vfazio avatar Sep 26 '24 18:09 vfazio