Use zero-argument super()

[EliahKagan]

This replaces 2-argument super(...) calls with the zero-argument super() form everywhere applicable:

• All uses of two-argument super in the code are replaced.
• All but one mention of two-argument super in comments are replaced.

I had omitted this from #1725 because it may require more technical attention to fully check than other code changes there, and that PR was mostly for comments and docstrings.

Exactly when super() can be used

Since super is in practice one of the most confusing features of Python, I've included this section to help in verifying the changes here, and also so that they could be reverified later. However, this section is optional.

As noted in output of help(super) (which is from this code, though the other documentation can be useful):

super() -> same as super(__class__, <first argument>)

__class__ is actually literal there--though the static __class__ cell should not be confused with the dynamic __class__ attribute--but it can also be thought of as standing for the name of the enclosing class (or narrowest enclosing class in the case of nested classes), which is actually what one typically writes in the two-argument form of super.

When the expression passed in place of __class__ refers to the (narrowest enclosing) class, and the expression passed in place of <first argument> is the name of the first parameter of the method (typically self in instance methods, or cls in class methods or __new__), the zero-argument form super() can be used instead with the same meaning.

The one place I couldn't replace it

The above-described situation applied everywhere in GitPython where super was actually used.

However, there are also comments in some places describing how something else could be done or how what was done could be done differently. In those, most showings of multiple-argument super were replaceable with super without changing what the code would mean and do if used in the place the comment pertained to. But then there is this, in git.objects.util.TraversableIterableObj:

GitPython/git/objects/util.py

Lines 625 to 658 in 6cef9c0

	def traverse(
	self: T_TIobj,
	predicate: Callable[[Union[T_TIobj, TIobj_tuple], int], bool] = lambda i, d: True,
	prune: Callable[[Union[T_TIobj, TIobj_tuple], int], bool] = lambda i, d: False,
	depth: int = -1,
	branch_first: bool = True,
	visit_once: bool = True,
	ignore_self: int = 1,
	as_edge: bool = False,
	) -> Union[Iterator[T_TIobj], Iterator[Tuple[T_TIobj, T_TIobj]], Iterator[TIobj_tuple]]:
	"""For documentation, see util.Traversable._traverse()"""
	"""
	# To typecheck instead of using cast.
	import itertools
	from git.types import TypeGuard
	def is_commit_traversed(inp: Tuple) -> TypeGuard[Tuple[Iterator[Tuple['Commit', 'Commit']]]]:
	for x in inp[1]:
	if not isinstance(x, tuple) and len(x) != 2:
	if all(isinstance(inner, Commit) for inner in x):
	continue
	return True
	ret = super(Commit, self).traverse(predicate, prune, depth, branch_first, visit_once, ignore_self, as_edge)
	ret_tup = itertools.tee(ret, 2)
	assert is_commit_traversed(ret_tup), f"{[type(x) for x in list(ret_tup[0])]}"
	return ret_tup[0]
	"""
	return cast(
	Union[Iterator[T_TIobj], Iterator[Tuple[Union[None, T_TIobj], T_TIobj]]],
	super(TraversableIterableObj, self)._traverse(
	predicate, prune, depth, branch_first, visit_once, ignore_self, as_edge # type: ignore
	),
	)

The actual super call in the code there is replaceable, and I replaced it, with zero-argument super(). But the comment--or, more precisely, the string being used as a comment--shows a call with Commit as the class name. I'm not sure if that's intended or not. If it is intended, maybe it represents code that would be written somewhere else. So I haven't changed that, though I'm willing to do so if the matter is clarified. It could be changed either in a new commit in this PR, or in a subsequent PR.

Other changes

There's some proofreading of the kind done in #1725 that I missed there and took care of here.

More significantly, TestBigRepoR.setUp contains:

GitPython/test/performance/lib.py

Lines 35 to 38 in 6cef9c0

	try:
	super(TestBigRepoR, self).setUp()
	except AttributeError:
	pass

And TestBigRepoRW.setUp analogously contains:

GitPython/test/performance/lib.py

Lines 67 to 70 in 6cef9c0

	try:
	super(TestBigRepoRW, self).setUp()
	except AttributeError:
	pass

In addition to replacing those two-argument super calls with zero-argument super(), I also removed the logic to catch and swallow AttributeError. As covered in more detail in the 9113177 commit message, this is unnecessary because unittest.TestCase defines an empty setUp method to allow this kind of special casing to be avoided, and it may be brittle because AttributeError could be raised from the actual code of a setUp method, due to the specific way the code to swallow it is written. However, if that latter effect, which would usually be strongly unwanted, is actually the goal, then this should be undone and a suitable comment and/or clarifying refactoring done.

What isn't included

It's unclear whether super calls in test classes' setUp and tearDown methods are always made in the places they are made because that is where superclass and sibling-class fixture acquisition and release behavior should occur. For example, usually, if setUp delegates via super to another class's setUp method at the beginning, and in a case where the corresponding tearDown method needs delegate via super to another class's tearDown method, then that tearDown call should be at the end, so that resource acquisition and release more often occurs in a temporally nested way that is easier to reason about and more often correct.

However, sometimes doing it differently is deliberate. Furthermore, setUp and tearDown methods are inherently weird, because, if a subclass's setUp calls a superclass's setUp and then fails with an exception, then tearDown won't run. (Also, in situations where tearDown does run, an exception in one tearDown method may still prevent other MRO classes' tearDown methods from running. Unlike the first issue, for this there isn't really any perfect thing to do. But I think the framework still makes it somewhat harder than necessary to reason about when the tearDown calls are manually written rather than arranged and implied by the setUp logic.)

For this reason, I am unsure if it is really worthwhile to try to improve ordering. If improvements are to be made, it might be best to have only setUp methods and have them register cleanup logic by calling addCleanup--or, perhaps better where applicable, enterContext, though a backport of it for Python <3.11 would be needed. Unlike tearDown methods, cleanup scheduled with addCleanup, either directly or through enterContext, is deterministically performed, in the opposite of the order it was added, even if setUp failed with an exception after the addCleanup call.

[Byron]

Thanks so much for working your way through this codebase of nearly 26k lines of code! I can't thank you enough.

If it is intended, maybe it represents code that would be written somewhere else. So I haven't changed that, though I'm willing to do so if the matter is clarified. It could be changed either in a new commit in this PR, or in a subsequent PR.

I definitely don't remember what the intention originally was, but I can absolutely believe that GitPython tried to be very clever with inheritance. Thus I suggest to find a test for this case or see if adding one yields desirable results. From there it should be clear what can be done (which might as well involve changing the doc-string).

For this reason, I am unsure if it is really worthwhile to try to improve ordering. If improvements are to be made, it might be best to have only setUp methods and have them register cleanup logic by calling addCleanup--or, perhaps better where applicable, enterContext, though a backport of it for Python <3.11 would be needed. Unlike tearDown methods, cleanup scheduled with addCleanup, either directly or through enterContext, is deterministically performed, and in the opposite of the order it was added.

To me this reads like the way super(…) is used here can stay until there is an incentive to use addCleanup(), which seems like a superior way of doing this along with the guarantee that the tear-down will happen as well.

[EliahKagan]

Thanks so much for working your way through this codebase of nearly 26k lines of code! I can't thank you enough.

You're welcome! but I feel this praise is somewhat undeserved. As noted in #1725 (comment), the way I've taken in the code is closer to snorkeling along the surface than a deep dive. Over time, however, I expect my knowledge and understanding of the code to deepen further.

For the change here in this PR, broad familiarity with the codebase was less relevant than in #1725, because the code that needed to be checked to verify correctness was always fairly nearby: I just needed to check, in each occurrence of super passing two arguments, that it occurred in a method, that the first argument was the name of the (nearest enclosing) class, and that the second argument was the method's first parameter.

To me this reads like the way super(…) is used here can stay until there is an incentive to use addCleanup(), which seems like a superior way of doing this along with the guarantee that the tear-down will happen as well.

Another option, which would sort of make this irrelevant, would be to convert all the tests to be pure pytest tests. Most tests in the project use unittest, being implemented as methods of classes that inherit (indirectly) from unittest.TestCase. A small number are pure pytest tests, but not most, and currently none use pytest fixtures. pytest fixtures take care of cleanup as well as unittest.TestCase.addCleanup. A pytest fixture that requires cleanup can ensure it is performed by either (a) depending on another fixture that provides it, or (b) being implemented as a generator that does cleanup after the yield (sort of like how one writes a @contextlib.contextmanager-decorated function, except without writing the try-finally oneself).

I already plan to make some of the tests in test_util.py pure pytest tests, so that the xfail markings will be granular, eliminating all XPASSes and making immediately clear from the test code itself exactly which parameter sets produce failure on which tests. (This is what I meant in #1725 under "What is omitted".) I would definitely not try to shoehorn a big design change affecting the entire test suite into that change. Nor do I have a drive to break things that are already working well. However, perhaps over time complete conversion of the test suite to pure pytest tests, with the further pytest features that this facilitates, would become natural.

[Byron]

However, perhaps over time complete conversion of the test suite to pure pytest tests, with the further pytest features that this facilitates, would become natural

Yes, that sounds like the way to go. Keep working what's working already, and if opportunities arise to improve it, probably by using pytest instead, that can be done.
Since you are working with the code the most these days, it's really about what seems most useful to you.