Repository: gitpython-developers/GitPython

Issue:
Unclear why iter_items should be favored over list_items
In [`git.util.IterableObj`](https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1237-L1271), the [`list_items`](https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1248-L1261) method recommends that [`iter_items`](https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1263-L1271) be used instead, and begins to explain why, but it appears the actual explanation was never written:

https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1255-L1256

The deprecated [`git.util.Iterable`](https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1201-L1233) class has the same note in its [`list_items`](https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1209-L1224) docstring:

https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1218-L1219

In both cases, the subsequent non-blank line in the docstring documents what the `list_items` method 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 whole `IterableObj.list_items` docstring:

https://github.com/gitpython-developers/GitPython/blob/4023f28660e3a78ca9a83f03ca10b39780d32b0e/git/util.py#L1250-L1258

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 `Iterable` class 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_items` docstrings 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_items` serves to recommend it. So I don't think I could propose a specific improvement to that documentation without insight into what was originally intended.

Discussion:

Patch:
diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index 651d9535..49dfedf9 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -1401,7 +1401,7 @@ class Submodule(IndexObject, TraversableIterableObj):
             pc = repo.commit(parent_commit)  # Parent commit instance
             parser = cls._config_parser(repo, pc, read_only=True)
         except (IOError, BadName):
-            return iter([])
+            return
         # END handle empty iterator
 
         for sms in parser.sections():
diff --git a/git/remote.py b/git/remote.py
index 4055dba2..98a421b3 100644
--- a/git/remote.py
+++ b/git/remote.py
@@ -130,7 +130,7 @@ def to_progress_instance(
     return progress
 
 
-class PushInfo(IterableObj, object):
+class PushInfo(IterableObj):
     """
     Carries information about the result of a push operation of a single head::
 
@@ -300,7 +300,7 @@ class PushInfoList(IterableList[PushInfo]):
             raise self.error
 
 
-class FetchInfo(IterableObj, object):
+class FetchInfo(IterableObj):
     """
     Carries information about the results of a fetch operation of a single head::
 
diff --git a/git/util.py b/git/util.py
index 0a5da7d7..5acc001f 100644
--- a/git/util.py
+++ b/git/util.py
@@ -1183,7 +1183,8 @@ class IterableList(List[T_IterableObj]):
 
 
 class IterableClassWatcher(type):
-    """Metaclass that watches."""
+    """Metaclass that issues :class:`DeprecationWarning` when :class:`git.util.Iterable`
+    is subclassed."""
 
     def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None:
         for base in bases:
@@ -1199,23 +1200,42 @@ class IterableClassWatcher(type):
 
 
 class Iterable(metaclass=IterableClassWatcher):
-    """Defines an interface for iterable items, so there is a uniform way to retrieve
-    and iterate items within the git repository."""
+    """Deprecated, use :class:`IterableObj` instead.
+
+    Defines an interface for iterable items, so there is a uniform way to retrieve
+    and iterate items within the git repository.
+    """
 
     __slots__ = ()
 
     _id_attribute_ = "attribute that most suitably identifies your instance"
 
     @classmethod
-    def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Any:
+    def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Any:
+        # return typed to be compatible with subtypes e.g. Remote
+        """Deprecated, use :class:`IterableObj` instead.
+
+        Find (all) items of this type.
+
+        Subclasses can specify ``args`` and ``kwargs`` differently, and may use them for
+        filtering. However, when the method is called with no additional positional or
+        keyword arguments, subclasses are obliged to to yield all items.
+
+        :return: Iterator yielding Items
         """
-        Deprecated, use IterableObj instead.
+        raise NotImplementedError("To be implemented by Subclass")
+
+    @classmethod
+    def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Any:
+        """Deprecated, use :class:`IterableObj` instead.
+
+        Find (all) items of this type and collect them into a list.
 
-        Find all items of this type - subclasses can specify args and kwargs differently.
-        If no args are given, subclasses are obliged to return all items if no additional
-        arguments arg given.
+        For more information about the arguments, see :meth:`list_items`.
 
-        :note: Favor the iter_items method as it will
+        :note: Favor the :meth:`iter_items` method as it will avoid eagerly collecting
+            all items. When there are many items, that can slow performance and increase
+            memory usage.
 
         :return: list(Item,...) list of item instances
         """
@@ -1223,15 +1243,6 @@ class Iterable(metaclass=IterableClassWatcher):
         out_list.extend(cls.iter_items(repo, *args, **kwargs))
         return out_list
 
-    @classmethod
-    def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Any:
-        # return typed to be compatible with subtypes e.g. Remote
-        """For more information about the arguments, see list_items.
-
-        :return: Iterator yielding Items
-        """
-        raise NotImplementedError("To be implemented by Subclass")
-
 
 @runtime_checkable
 class IterableObj(Protocol):
@@ -1246,13 +1257,30 @@ class IterableObj(Protocol):
     _id_attribute_: str
 
     @classmethod
-    def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> IterableList[T_IterableObj]:
+    @abstractmethod
+    def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Iterator[T_IterableObj]:
+        # Return-typed to be compatible with subtypes e.g. Remote.
+        """Find (all) items of this type.
+
+        Subclasses can specify ``args`` and ``kwargs`` differently, and may use them for
+        filtering. However, when the method is called with no additional positional or
+        keyword arguments, subclasses are obliged to to yield all items.
+
+        For more information about the arguments, see list_items.
+
+        :return: Iterator yielding Items
         """
-        Find all items of this type - subclasses can specify args and kwargs differently.
-        If no args are given, subclasses are obliged to return all items if no additional
-        arguments arg given.
+        raise NotImplementedError("To be implemented by Subclass")
+
+    @classmethod
+    def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> IterableList[T_IterableObj]:
+        """Find (all) items of this type and collect them into a list.
+
+        For more information about the arguments, see :meth:`list_items`.
 
-        :note: Favor the iter_items method as it will
+        :note: Favor the :meth:`iter_items` method as it will avoid eagerly collecting
+            all items. When there are many items, that can slow performance and increase
+            memory usage.
 
         :return: list(Item,...) list of item instances
         """
@@ -1260,16 +1288,6 @@ class IterableObj(Protocol):
         out_list.extend(cls.iter_items(repo, *args, **kwargs))
         return out_list
 
-    @classmethod
-    @abstractmethod
-    def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Iterator[T_IterableObj]:  # Iterator[T_IterableObj]:
-        # Return-typed to be compatible with subtypes e.g. Remote.
-        """For more information about the arguments, see list_items.
-
-        :return: Iterator yielding Items
-        """
-        raise NotImplementedError("To be implemented by Subclass")
-
 
 # } END classes
 


Test patch:
diff --git a/test/test_submodule.py b/test/test_submodule.py
index 4dc89f98..993f6b57 100644
--- a/test/test_submodule.py
+++ b/test/test_submodule.py
@@ -688,6 +688,18 @@ class TestSubmodule(TestBase):
         # gitdb: has either 1 or 2 submodules depending on the version.
         assert len(nsm.children()) >= 1 and nsmc.module_exists()
 
+    def test_iter_items_from_nonexistent_hash(self):
+        it = Submodule.iter_items(self.rorepo, "b4ecbfaa90c8be6ed6d9fb4e57cc824663ae15b4")
+        with self.assertRaisesRegex(ValueError, r"\bcould not be resolved\b"):
+            next(it)
+
+    def test_iter_items_from_invalid_hash(self):
+        """Check legacy behavaior on BadName (also applies to IOError, i.e. OSError)."""
+        it = Submodule.iter_items(self.rorepo, "xyz")
+        with self.assertRaises(StopIteration) as ctx:
+            next(it)
+        self.assertIsNone(ctx.exception.value)
+
     @with_rw_repo(k_no_subm_tag, bare=False)
     def test_first_submodule(self, rwrepo):
         assert len(list(rwrepo.iter_submodules())) == 0