There's a flood of incoming code from contributors like me, but the code lies there, around, unused for months or even years.
It's great we have so many contributors to Minetest, but I'm afraid a lot of contributions are essentially going to waste.
The number of open PRs has been steadily increasing over the years. Contributors are posting PRs faster than core devs can react to them. This is very bad. There are many potential contributors but only a small handful of people with merge rights. Therefore, all PRs have to go through a very tiny bottleneck.
There are also many PRs for which, frankly, I believe they have no snowball's chance in hell to ever get merged. That's not the problem, the problem is that such PRs are still kept open for some reason. Zombie PRs. Oldest open PRs are from 2016, yes, 2016, and they are kept open for some reason.
Another problem with letting PRs rot for so long is that it naturally becomes harder and harder to merge them as time progresses. The much-feared label “rebase needed” has effectively killed quite a few PRs. As a contributor, you need a VERY long breath, otherwise you have no chance. Many contributors just disappear before a PR is merge-ready. That's so frustrating! There are a lot of hard-working contributors whose work basically goes to waste, effectively.
Paramat keeps complaining that “dev time” is very limited, and that might be true. But it is also true that there are a LOT of people who actively want to contribute to Minetest. It's just that those people don't happen to be coredevs. So this might be huge missed opportunity for Minetest.
I think something needs to happen. The PR problem will continue to become worse and worse, with so many PRs. Contributors are given false hopes that their contribution has a chance of getting merged, but in reality, the chances are not very good (they compete with >100 other PRs).
I know that reviewer time is a VERY limited resource, but I like to discuss about whether there are methods to deal with PRs more effectively. This is important, precisely *because* time is very limited.
But I think, currently, the way how PRs are dealt with is extremely time-inefficient. I have identified a couple of problems:
- A LOT of time is wasted with talking about irrelevant details before considering the bigger picture. If a PR is unsound on principle, it's pointless to discuss details
- PRs are very rarely rejected. Instead, they are just kept alive indefinitely and are forgotten, but still technically open. Rejection of PRs needs to be done much more aggressively. PRs for which there is core dev consensus that it is not for MT, should be closed. Low-quality PRs for which the author is unwilling or unable to make the requested improvements should be closed. Ancient PRs for which the author has disappeared a long time ago should be closed (consider adding a label “for historic interest”).
- Coding style is analyzed before the overall soundness of the PR is analyzed. It's pointless to discuss coding style if it turns out a PR is garbage
- Code rot: The longer a PR lays around unmerged, the harder it becomes to merge it. Some cases are so bad, it's easier for the PR author to throw the PR away and start over …
- “Rebase needed”: An ugly side effect of code rot. Many contributors cop out when this happens. This label can be a death sentence to a PR. It's also very frightening. You are basically asking the PR author to risk their repo integrity.
- Ridiculous coding style rules: Honestly, I hate MT's coding style, it's just a waste of time to deal with it every time. The core devs enforce the coding style religiously, even if it makes the code uglier. OK, this is just my personal opinion, but it just annoys me so much! :D
- No real priotization of PRs. Most PRs are treated equally, and it's difficult to navigate the jungle.
- There are just too many open PRs in general. This means many good PRs just drown in all the noise.
- “Ping-pong”: Basically, core devs do a superficial review (coding style, etc.) and ignore the bigger picture, then disappear for good. It is implied the author must react. Nothing will happen until the PR author reacts, even if it's just about a typo. Then the game repeats, the core devs review another small thing until they bump in another minor problem, then immediately stop the review again, and disappear again.. I call it “ping-pong” because each party is throwing the ping-pong ball to the other side. I think ping-ponging is EXTREMELY time-efficient, because there's always a delay until the other party is able to react (due to timezones). And the PR author is forced to be active at multiple points in time to basically only do trivial fixes, just to hit the ball again! But it would be much more efficient if the PR author receives all complaints at once, so they can be all fixed at once.
I have a couple of suggestions to reorganize the dealing with PRs:
- Shut down off-topic discussions when they arise.
- Be more aggreesive, and faster with rejecting PR that don't go anywhere. Make actual decisions instead of keeping every PR open for all eternity. Don't delay a decison. If you can't come to a decision for months, close the PR.
- Have actual rules / a procedure for rejection so everything is more straight-forward. This also helps contributors and will prevent tears because nobody can't say they haven't been warned.
- Get rid of the stupid coding style rules. The 80 char line length is annoying af. IMO the only thing a coding style needs is indents, everything else is BS.
- Hold back your review before you can do it more in-depth. If you expect a reaction from the PR author, it's way more efficient when you give them all complaints at once. Avoid useless mini-reviews
- Start to priotize PRs. Sort by complexity. Use multiple priority levels if you must. Deal with high-impact high-complexity PRs first. Those are the PRs that will attract code rot first.
- If you don't have time for a proper (!) review, don't bother. It's of no help for the PR author if there are only superficial or incomplete reviews. Come back when you have time.
- Consider adding more reviewers.