GeezerVison is showing github.com — [tracking] Unbreaking v3.1.15 (yanked) · Issue #1223 · gitpython-developers/GitPython
[tracking] Unbreaking v3.1.15 (yanked) #1223DescriptionWith 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
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 Activitymuggenhor commented on Apr 22, 2021 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:
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. Reacted by Sebastian Thiel and phlax Byron commented on Apr 22, 2021 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 commented on Apr 22, 2021 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 commented on Apr 23, 2021 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 commented on Apr 23, 2021 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 commented on Apr 23, 2021 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 commented on Apr 23, 2021 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! Byron commented on Apr 24, 2021 Thanks for chiming in @Yobmod.
It's an interesting situation GitPython is in now as it appears it breaks CIs of users who do use typechecking if its own types fail, which seems to be what this issue is about.
Checking our own types means everything written in future needs special annotation to ignore typecheck failures, but on the other hand I'd say this is preferable if it gives the chance of finding genuine bugs. To my mind this is why types were introduced in the first place.
My stance here is to do type checking, ideally with all the mainstream checker tools available, and keeps the type system happy knowing that among false positives there are genuine bugs to discover. This keeps users CI's happy, too, at the cost of forcing contributions to deal with it. A cost I am happy to take in the name of the maintainability of this project.
An alternative I see is to remove all types and continue without forever which seems like a 180 degree turn.
It could be set up to to use separate, less strict config (ie doesnt require types for all functions etc) than the dev one?
This seems like middle grounds are possible, and without knowing anything about mypy I'd hope that we can bring something like it to fruition before the next release. Byron commented on May 13, 2021
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|