Repository navigation
Unclear why iter_items should be favored over list_items #1775
Description
Activity
Thanks for bringing this up!
Solely on the basis of 'knowing myself a little' I'd think that the continuation would be a general listing of advantages of iterators. If
iter_itemsis indeed implemented as iterator, it should be worth following up on finishing the note as the latency of collecting all items could be high, depending on the item at hand. Nowadays I'd argue thatlist_items()shouldn't exist in the first place, but it's too late for that now.- added 2 commits that reference this issue
on Dec 22, 2023 If
iter_itemsis indeed implemented as iteratorI would say it is implemented as an iterator. The more detailed picture is:
- Hopefully code outside GitPython satisfies the documented contract, and GitPython can't be responsible for ensuring that.
- If anything implements the deprecated
git.util.Iterableanymore, it's code outside GitPython. iter_itemsis abstract ingit.util.IterableObj, which introduces it.iter_itemsexplicitly raisesNotImplementedErrorinPushInfoandFetchInfo. Those concrete classes declare themselves to implementIterableObjyet, in effect, decline to implement this abstract method. However, because those classes don't implementlist_items, whose base-class implementation usesiter_items, this is not a reason to avoid recommendingiter_itemsoverlist_itemsin the base class docstrings.- Other classes that implement
IterableObjhave concrete implementations ofiter_itemsthat return iterators: either their own, or those introduced by an intermediate base class. I don't think I missed anything while looking into this. - The other
iter_itemsmethod in the codebase belongs toSymbolicReference, and it returns an iterator.SymbolicReferencedoes not inherit fromIterableObj, nor from any other classes, except the implicit inheritance fromobject(not to be confused with GitPython'sObject). I wonder if it would be beneficial to explain its relationship further in its class docstring; the description "Special case of a reference that is symbolic" might lead readers to think it is a subclass of some class representing a more general case, or that it is intended to be a subclass ofReference(which does implementIterableObj). But if I understand correctly, this is independent of any considerations in howIterableObjand its methods should be documented.
I've opened #1780 to propose some
IterableObjanditer_itemsrelated improvements, including filling in the missing information about whyiter_itemsshould be preferred tolist_itemsalong the lines of what you've said here.Reacted by Sebastian Thiel
In
git.util.IterableObj, thelist_itemsmethod recommends thatiter_itemsbe used instead, and begins to explain why, but it appears the actual explanation was never written:GitPython/git/util.py
Lines 1255 to 1256 in 4023f28
The deprecated
git.util.Iterableclass has the same note in itslist_itemsdocstring:GitPython/git/util.py
Lines 1218 to 1219 in 4023f28
In both cases, the subsequent non-blank line in the docstring documents what the
list_itemsmethod itself returns, so the continuation truly is missing. The intended continuation does not seem to me to be something that can be inferred from the docstring as a whole. For example, here's the wholeIterableObj.list_itemsdocstring:GitPython/git/util.py
Lines 1250 to 1258 in 4023f28
It may seem odd that, unlike #1712, I did not notice this when working on #1725. But I think the reason is not that the docstrings had made sense to me at that time, but instead that I had noticed the more minor (really, almost trivial) issue that perhaps the first paragraph should be split so that only its first sentence would be its summary line, decided to return to it later to consider that further, and then forgot about it.
The text "Favor the iter_items method as it will" appears to have been present, and its continuation absent, for as long as the surrounding code has been in GitPython. It was introduced in f4fa1cb along with the
Iterableclass itself.In general, it may sometimes be preferable to obtain an iterator rather than a list or other sequence because it may not be necessary to materialize a collection just to iterate over it, and because unnecessary materialization can sometimes increase space usage. On the other hand, materialization guards against mutation of the original collection during iteration. But these are completely general ideas, not informed by the
list_itemsdocstrings nor even by any consideration specific to GitPython.My guess is that the docstring intended to say something more specific, or at least to identify which general benefit of
iter_itemsserves to recommend it. So I don't think I could propose a specific improvement to that documentation without insight into what was originally intended.