Open vfazio opened 3 weeks ago
How much of this can be changed without changing the API? Looks like replacing tuple with Iterable should be pretty straightforward?
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
.
Sounds good, I hope it's not too much to ask of you to also do the tuple -> Iterable conversion?
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?
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
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
: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 asIterable[Union[int, str]]
Similarly,
dict
is a bit too strict when we're just requiring aMapping
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 genericMapping
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 covariantMapping
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:We only require that there be a
keys()
anditems()
method implemented on thedict
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).offset: dict[int]
ornamed: dict[str]
. Splitting these out allows checking for overlap from the resolved line names to the offsets.