On Mon, Jul 27, 2020 at 11:19:46AM +0200, Erik Skultety wrote: > On Mon, Jul 27, 2020 at 08:32:29AM +0200, Philipp Hahn wrote: > > Hello, > > > > Am 25.07.20 um 23:45 schrieb Philipp Hahn: > > > Am 27.04.20 um 15:44 schrieb Philipp Hahn: > > >> I'm working on adding PEP 484 type hints > > >> <https://www.python.org/dev/peps/pep-0484/> to the Python binding of > > >> libvirt. > > ... > > > I just opened a merge request > > > <https://gitlab.com/libvirt/libvirt-python/-/merge_requests/9> for my > > > code at <https://gitlab.com/pmhahn/libvirt-python/-/tree/typing>. > > > > While working on that I stumbled over some annoyances in the current > > Python API: There are many methods which return a List[int], for example: > > virDomainGetInfo > > virDomainGetState > > virDomainGetControlInfo > > virDomainGetBlockInfo > > virDomainGetJobInfo > > virStoragePoolGetInfo > > virStorageVolGetInfo > > virStorageVolGetInfoFlags > > > > There are more function returning similar information as plain List: > > virNodeGetSecurityModel > > virNodeGetSecurityModel > > virDomainGetSecurityLabelList > > virDomainBlockStats > > virDomainInterfaceStats > > virDomainGetVcpus > > virNodeGetCPUMap > > > > The worst offender is `virNodeGetInfo`, which returns `Tuple[str, > > int*7]`: The problem for type annotation is that `List` is unlimited in > > the number of elements, that is you cannot specify the number of entries > > the list must or will contain. > > Also all elements of a list must have the same (super-)type; for "str" > > and "int" of "virNodeGetInfo()" that would be "Any", which is not very > > useful here. > > > > A better type for those `List`s would be `Tuple`, which has a fixed > > length and can have different types for each position. > > > > But that would be an API change: In most cases users of those functions > > should not notice the difference as indexing tuples or lists is the same. > > It would break code where someone would do something like this: > > ret = virFunction_returning_list() > > ret += [1, 2, 3] > > I would argue that ^this was never the intended way of using the > returned object and would lean towards using a named tuple, since like > you've pointed out it would be a transparent change in the vast majority > of cases. However, since we don't document anywhere how the return > value should be treated, from that perspective it's still valid to > change the returned list for whatever purposes and therefore we are > breaking the API contract, which we can't do even though the change > itself is reasonable. I think the above example is just plain crazy. While it may be technically possible, I don't think that example is a compelling usage to justify not doing the tuple change. Even if it does break some app (I very much doubt it will), it'll still be a net win for all the other sane apps to change to using a named tuple. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
