Better document IterableObj.iter_items and improve some subclasses by EliahKagan · Pull Request #1780 · gitpython-developers/GitPython (original) (raw)

Fixes #1775
Fixes #1779

This includes improvements to docstrings, and also other clarity improvements such as reordering methods, in both git.util.IterableObj and the deprecated git.util.Iterable class and its associated metaclass.

For #1775, it fills in the missing information. It also rewords the surrounding information, has the less recommended list_items methods' docstrings refer to the more recommended iter_items methods (instead of the other way around), and makes some further changes along those lines detailed in dfee31f.

I think that and the other changes in this pull request, documented in commit messages and the above-linked issues this closes, make sense to do together. For #1779, I added tests for some surrounding behavior that did not appear to be under test, as well as extending one of them to be a regression test for that minor bug, before fixing it.

I opened #1779 as its own issue rather than documenting here so that, if it turns out that both it and the bigger changes related to #1775 need to be discussed, the discussions are less likely to clash. (But I have no preference as to where such discussion should take place.)

There are some possible further changes to git.util.IterableObj and git.util.Iterable that I did not include here because I am not sure if they are wanted, but which I'd be pleased to open another pull request for (or to add to this pull request if preferred):

• I think it would make sense to put the class that is actually used in GitPython, and that other people should use, first. That is, I think IterableObj should be defined before Iterable. I can see why it's not done that way now: defining Iterable first allows its associated metaclass to be defined next to it, and also for Iterable and IterableObj to be defined next to each other, which may be beneficial since it may make it less likely for their similar docstrings to go out of sync.
• In IterableObj and Iterable, I think it would be okay for only the methods that are most recommended to give full details about their use, and others to refer to those methods. I made this change partially, moving information that was only in the list_items methods to the iter_items methods. But I propose going further and having the docstrings in the deprecated Iterable class refer to those in IterableObj for their detailed information, at least regarding the effect of omitting or passing extra arguments. I did not do this here because I don't know why the docstrings were near-duplicated in the first place, and perhaps there is a good reason for them to be that way. There would also be the question of whether these comments should be made different or one of them removed, given that the return types they're referring to are not the same:

  def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Any:
  # return typed to be compatible with subtypes e.g. Remote
  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.

If the second of those suggestions it to be done, then the first can be done too with little to no disadvantage, since it would no longer be necessary to keep their docstrings in sync.