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

[vfazio]

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:

python/typing#445

python/typing_extensions#5

hauntsaninja/useful_types#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.

[brgl]

How much of this can be changed without changing the API? Looks like replacing tuple with Iterable should be pretty straightforward?

[vfazio]

How much of this can be changed without changing the API? Looks like replacing tuple with Iterable should be pretty straightforward?

The tuple -> Iterable change is backwards compatible and straightforward

I think this change has immediate benefits, mostly because there are more types that conform to that interface and it's a pretty simple change. I thought about changing it in my series, but decided it's best as it's own commit, but it would definitely affect #97.

The dict -> CovariantMapping change is backwards compatible but less straightforward

After thinking about it, trying to loosen dict to some Mapping seems less beneficial because the number of variants of dict is limited. Someone would have had to hand craft a class to conform to the interface. Classes like OrderedDict/defaultdict are already subclasses of dict so would be acceptable arguments. However, stdlib's ChainMap can't be despite it having the appropriate methods used internally in the method.

I'd probably make the case that instead of trying to loosen the dict type further we'd be better off breaking the interface at some point in the future to cover some of those bullet points.

---

Naive impl:

class RequestConfig:
    _offsets: dict[int, LineSettings | None] = {}
    _names: dict[str, LineSettings | None] = {}

    def _insert_name(self, name: str, value: LineSettings | None, raise_err: bool = True) -> None:
        if raise_err and name in self._names.keys():
            raise KeyError(f"Line name {name} already requested.")
        self._names[name] = value

    def _insert_offset(self, offset: int, value: LineSettings | None, raise_err: bool = True) -> None:
        if raise_err and offset in self._offsets.keys():
            raise KeyError(f"Line offset {offset} already requested.")
        self._offsets[offset] = value

    def _insert_iterable(self, it: Iterable[int | str], value: LineSettings | None, raise_err: bool = True) -> None:
        for item in it:
            if isinstance(item, str):
                self._insert_name(item, value, raise_err=raise_err)
            if isinstance(item, int):
                self._insert_offset(item, value, raise_err=raise_err)
            else:
                raise ValueError(f"Invalid argument type for key {item}")

    def insert(self, key: Iterable[int | str] | int | str, value: LineSettings | None, raise_err: bool = True) -> None:
        if not isinstance(key, (Iterable, int, str)):
            raise ValueError(f"Invalid argument type for key {key}")
        if isinstance(key, str):
            self._insert_name(key, value, raise_err=raise_err)
        if isinstance(key, int):
            self._insert_offset(key, value, raise_err=raise_err)
        else:
            self._insert_iterable(key, value, raise_err=raise_err)

    @staticmethod
    def from_dict(old_argument_style: dict[Iterable[int | str] | int | str, LineSettings | None]) -> RequestConfig:
        """
        Construct a RequestConfig from old style arguments
        """
        req = RequestConfig()
        for k, v in old_argument_style.items():
            req.insert(k, v, raise_err=True)
        return req

I'm not totally sold on having to iterate through an iterable; callers could just as easily just call insert multiple times so that the exposed interface is simpler.

An object like this would make it easier to comb through the requested line names, resolve them to offsets and check to see if a duplicate line was requested simplifying some of that Counter logic we have in Chip.request_lines.

[brgl]

Sounds good, I hope it's not too much to ask of you to also do the tuple -> Iterable conversion?

[vfazio]

Sounds good, I hope it's not too much to ask of you to also do the tuple -> Iterable conversion?

Nope, I can do that as well. I assume you want that as part of my series? Or do you want that as a separate patch?

[brgl]

Sounds good, I hope it's not too much to ask of you to also do the tuple -> Iterable conversion?

Nope, I can do that as well. I assume you want that as part of my series? Or do you want that as a separate patch?

Whatever works best for you

[vfazio]

I'm doing this as part of the series in #97. Taking me longer to clean that up than i'd like.