libvirt-python: API change List → (Named)Tuple?

[Daniel P. Berrangé]

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 :|