[tracking] Unbreaking v3.1.15 (yanked)

[Byron]

With the release of v3.1.15 a few breaking bugs were discovered. This caused yanking the version ~14h later.
Here we collect the kind of breakage and track their fixes to allow for a new release.

Until this issue is resolved no new releases can be made.

Issues

• GitCommand missing stderr #1221 GitCommand missing stderr - fixed with Fix missing stderr when the progress parameter of _clone is None #1224
• GitCommand errors are stringifying bytes in 3.1.15 #1220 GitCommand errors are stringifying bytes in 3.1.15 - fixed with Revert compiling GitCommand shell messages #1222
• Python 3.5 failing with AttributeError: module 'os' has no attribute 'PathLike' #1219 Python 3.5 to 3.8 failing with AttributeError: module 'os' has no attribute 'PathLike'
  • Even though it cropped up after the release CI doesn't yet pick it up. Typing should be evaluated to catch this.

Causes

My reviews aren't good enough and heavily rely heavily on the test-suite to pass. The latter, however, is far from complete, and the lack of static types makes it hard for tooling to expose issues at compile time.

Solutions

Please chime in if you have ideas on how to prevent breaking releases in future.

CC: @muggenhor @jmcgill298

[muggenhor]

I'm not sure how exactly to (quickly) address the lack of completeness of the test-suite. But I've got some thoughts on both the test suite and the process of how these bugs managed to slip in:

• your quick response to yank this release is really appreciated and significantly helps to reduce the impact of this in practice for my use cases
• the original PRs was one big massive commit that combined different kinds of refactor work, thus making it next to impossible for any reviewer to review. (FYI: I define a refactoring as a single kind of change to the structure of a program that should not affect behavior at all.) As a result some behavioral changes slipped through. In PRs I'm a reviewer on I often require authors to split up there changes into a "single logical change" per commit. E.g. adding the type annotations should probably not have been combined with moving the locations of function calls. (This might not have prevented the bug from slipping through, but it would have made my bisection for GitCommand missing stderr #1221 immediately lead to the fix)

About the test suite:

• GitCommand missing stderr #1221 this was caught relatively quickly when I re-ran the test suite of hopic, so there may be some inspirational test cases in there
• Having a TDD policy, of requiring new tests, for feature/bugfix PRs may help increase test coverage over time
• This may just be me and a reflection of what I work with more often, but I find pytest much easier and more convenient to write tests in than pyunit. So it took me more time to fit the test case of test(clone): verify stderr for a failing clone into a non-empty dir #1225 in pyunit than to write the basic test itself.

And unfortunately none of this will be an instant guarantee to reduce the amount/severity of bugs that go undetected prior to a release below a certain threshold. At best any of these changes will, over time, slowly start to improve this situation.

[Byron]

Fantastic, thanks for sharing your thoughts! Here is some my affirmative comments about all of them.

General

your quick response to yank this release is really appreciated and significantly helps to reduce the impact of this in practice for my use cases

Good to hear, I will keep doing that.

the original PRs was one big massive commit that combined different kinds of refactor work, thus making it next to impossible for any reviewer to review.

Agreed, that should have been a red flag, even for myself when I notice I am getting 'sloppy' just to get done with it.

In PRs I'm a reviewer on I often require authors to split up there changes into a "single logical change" per commit. E.g. adding the type annotations should probably not have been combined with moving the locations of function calls.

I couldn't agree more. I admit that I didn't even see the changes that weren't related to typing. Furthermore I tend to want to close PRs quickly so try to avoid asking for too many changes especially when I sloppily think 'Nah, it will be fine' :D. It's definitely one of my biases to be aware of.

To help with this, I updated the contribution guidelines to especially ask for many small steps, one per commit.

Test Suite

Having a TDD policy, of requiring new tests, for feature/bugfix PRs may help increase test coverage over time

This is already part of the contribution guidelines, but I find myself not enforcing it when I think it's hard to write a test for it along with my bias to not try to ask for too many changes to get things merged more quickly. Thus far I could trust the existing test suites, but as always, the devil is in the details.

This may just be me and a reflection of what I work with more often, but I find pytest much easier and more convenient to write tests in than pyunit. So it took me more time to fit the test case of #1225 in pyunit than to write the basic test itself.

It's in my very interest to provide a code base that is friendly to contributors, particularly when it comes to running and writing tests. Thus I take your preference as more general truth and hope there is a way to use both test frameworks in the same codebase if that's feasible at all.

If you have ideas to reduce the resistance when writing tests I would be more than happy to accept PRs right away.

[Byron]

In theory, a new release cut be made now, but I would like to explicitly invite @c4xuxo in case they would be able to contribute a failing test to reproduce #1219 so it can be addressed. Doing so would probably mean to either drop python 3.5 support officially or to be more explicit about the patch level required to run this codebase.

I will wait a few days for this to settle and then cut a release.

[muggenhor]

After your last message I gave #1219 a more detailed look. Seems that's a problem for 3.5, 3.6, 3.7 and 3.8. The code only seems to work with 3.9.

[Byron]

With #1226 merged we can at least say that mypy is now usable with GitPython, even though there may be other typecheckers that might still not work (and thus break some pipelines out there)?

Do you think that's a good basis for cutting a new release, or would you rather wait until more typechecker support was added?

[muggenhor]

I've seen things that I'm sure are wrongly annotated types but only in internal code (e.g. the -> 'metaclass' declaration of with_metaclass is just bullshit). A runtime checker will probably find at least some of those. But unless some pipeline out there is doing code injection (into GitPython) I don't see why it would break as a result.

So I think it's a good enough, if a bit ugly, basis for doing a new release. You'll probably want to mention it as a known issue in the change log though.

[Yobmod]

Hi all,

Thanks for the fix of my mistake. I've been away on work, so not been able to get online until now.
I had expected the CI to catch import problems like that, with any test that called any function using pathlike triggering it. My fault for putting it as a forward refererence maybe, or thinking at least one test must be calling it.

I'm hesitant to suggest adding mypy to the CI, as it would force all contributers to type everything or add type ignore comments when mypy cannot check it (as the mypy.ini gets more strict). It could be set up to to use separate, less strict config (ie doesnt require types for all functions etc) than the dev one?

I've seen things that I'm sure are wrongly annotated types but only in internal code (e.g. the -> 'metaclass' declaration of with_metaclass is just bullshit)

Yep. Mypy and/or pylance complained about anything i put as the return type except Any, hence the ignore and additional comment. Should probably just be TBD, in case mypy improves.

Thanks again!