Hi Jeen and everyone else,
I’m good with this, and it will make things more explicit.
It will affect turn around time though. You (Jeen) are the one to review 99% of my code. Sometimes I catch you in my evening or morning, but usually I will have to wait 12-24 hours for feedback at a minimum.
I would like to propose that we don’t need to switch platforms. Pinging on GitHub should be enough. And furthermore I propose a Cart Blanche to merge if no one has responded in 48 hours.
Fine with sticking with Github - just use what you need to get attention (and everybody else, help your fellow committers out and occasionally check).
As for merging after 48 hours, a couple of points: first of all, I'm fine with this as a rule for smaller PRs where it's obviously a fix and any feedback you're going to get is likely to be small stylistic stuff. However, for larger PRs that introduce broader changes (big new feature, or new interfaces/public classes, or just generally a lot of commits or a lot of files changed) I would suggest that we are a little more careful. In any case: let's make it a point to remind people, and if there is some urgency to get something done (for example because it blocks you) then do say so.
Of course I don't want to get in a situation either where every PR sits open for a week - that's not very motivating for the author, and also code grows stale and you get into more merge conflicts that way.
During the time that you were away, and James had taken over most of the coding, there weren’t really that many other people doing code review. Explicit approval wouldn’t have worked back then. Today we are in a better place wrt. resources. Though we are still far away from the bigger players who have multiple paid employees working on their open source projects.
Yeah, we're a small team and we don't always have time to spend on reviews for this project. This is also why I don't want to make this a formally enforced branch protection thing. I'm talking guidelines and sensible defaults here, but we're all adults.
Fixing things after they are merged is also mostly fine. We shouldn’t aim for it, but I still say it’s mostly fine. We also used to do this a bit more frequently before we got the current GitHub actions setup to detect all test failures automatically and quickly.
To me, the main issue is not so much catching bugs / test failures, it's more about encountering certain designs or code refactoring changes suddenly that I was unaware we had made and that catch me out (possibly because I didn't pay attention, but sometimes also because something was merged before I had a good look). And on the other side, as a PR author, I'm confident enough that I typically know what I'm doing, but I still find it motivating to know that others have at least glanced at my changes and give it the thumbs up.
Cheers,
Jeen
