Merge pull request #1780 from EliahKagan/iteritems

Better document IterableObj.iter_items and improve some subclasses

Author: Byron

git/objects/submodule/base.py

@@ -1401,7 +1401,7 @@ def iter_items(
             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():

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 @@ def raise_if_error(self) -> None:
             raise self.error
 
 
-class FetchInfo(IterableObj, object):
+class FetchInfo(IterableObj):
     """
     Carries information about the results of a fetch operation of a single head::
 

git/util.py

@@ -1183,7 +1183,8 @@ def __delitem__(self, index: Union[SupportsIndex, int, slice, str]) -> None:
 
 
 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,39 +1200,49 @@ def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None:
 
 
 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
         """
         out_list: Any = IterableList(cls._id_attribute_)
         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,30 +1257,37 @@ 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
         """
         out_list: IterableList = IterableList(cls._id_attribute_)
         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/test_submodule.py

@@ -688,6 +688,18 @@ def test_root_module(self, rwrepo):
         # 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