We need more official PR reviewers

Post Reply
User avatar
Wuzzy
Member
Posts: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

We need more official PR reviewers

by Wuzzy » Post

I think I have finally found the #2 reason* why Minetest development is so slow: We have a major shortage in reviewers with approval rights!

What I mean with that is that there is only a tiny amount of people who are allowed to review Minetest submissions (pull requests on GitHub) and also give an official approval. As far I know, only core devs have official approval rights. On the other hand, PRs are pouring in at an alarming rate, the number of open (!) PRs hasn't dropped below 100 for a very long time. Also, a lot of PRs are very, very old, but still neither accepted nor rejected. >100 open PRs is a problem, because older (but high-quality) PRs get forgotten fast and it's hard to keep track of all of them in your mind.

Now add 1 and 1 together: We have a tiny handful of official reviewers (=core devs) while simultanously Minetest is bombarded with new PRs. Now it's pretty obvious why progress is so slow. The reviewers are just being overwhelmed.

Therefore, I propose one simple thing: Add more people with official approval rights. These people can give an approval to PRs that has the same weight as a core dev's approval.

How to decide who to make an official reviewer? I don't really know, but obviously these should be skilled people who know what they're doing and are aware of the Minetest development rules and they should also be not be afraid of rejecting bad PRs. Don't read this proposal as a sneaky way to sneak in every possible PR into Minetest, no matter how crappy. It's more about speeding up the process which decides whether a PR gets green light or red light.

Anyway, I think it shouldn't be too hard to find good reviewers. Our community is actually quite large, surely there are some good people. Right?

I imagine if the amount of official reviewers increases, Minetest will be able to significantly deal with a lot of PRs faster, and eventually get the number of open PRs below 100, below 50, below 25, below 10. Once the number of simultanously open PRs is low, dealing with new PRs will also be much easier, since they all fit on a single page, reducing cognitive load drastically. And the danger of PRs dying of old age will hopefully also be much lower.

Overall, my hope is that development will be more efficient that way.



(* = What is the #1 reason, you ask? Simple: Most core devs have very limited time. I don't blame them, but that's a different problem anyway …)

deleted c0a803ab

Re: We need more official PR reviewers

by deleted c0a803ab » Post

That is a very good suggestion Wuzzy!
+1

User avatar
Hugues Ross
Developer
Posts: 72
Joined: Mon May 06, 2019 22:52
GitHub: Df458
Location: Kitchens, Pantries, etc.
Contact:

Re: We need more official PR reviewers

by Hugues Ross » Post

+1 from me as well.

Speaking personally, I would be interested in a position like this (although ultimately this is a decision for the coredevs, not myself). As someone with other hobbies and a day job, I could never offer the dedication and responsibility to the project that a coredev needs, but I could spare time for reviews in my areas of expertise if I felt like they had any tangible impact on the process.

I suspect there are plenty of others in the community who feel the same way.

User avatar
rubenwardy
Moderator
Posts: 6972
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy
Location: Bristol, United Kingdom
Contact:

Re: We need more official PR reviewers

by rubenwardy » Post

Oh, boy, wish I'd thought of this /s

The reason why this doesn't happen much is because it is hard to find people who are trusted, have the skills, and are willing to invest sufficient time. V-rob and pyrollo were added to the team last year, but the number of PRs is still mostly unchanged.

Anyone can suggest anyone to become a core developer to celeron55. There is also the possibility of people being given review rights over certain areas, like a more specialised core dev. For example, we previously had someone who had the ability to approve and merge documentation PRs
Renewed Tab (my browser add-on) | Donate | Mods | Minetest Modding Book

Hello profile reader

sfan5
Moderator
Posts: 4094
Joined: Wed Aug 24, 2011 09:44
GitHub: sfan5
IRC: sfan5
Location: Germany

Re: We need more official PR reviewers

by sfan5 » Post

What I'm missing myself currently is another coredev who is reasonably active on IRC, which can discuss ideas with me, give a second opinion, etc.

What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
Mods: Mesecons | WorldEdit | Nuke & Minetest builds for Windows (32-bit & 64-bit)

User avatar
v-rob
Developer
Posts: 970
Joined: Thu Mar 24, 2016 03:19
GitHub: v-rob
IRC: v-rob
Location: Right behind you.

Re: We need more official PR reviewers

by v-rob » Post

rubenwardy wrote:
Fri Apr 02, 2021 22:22
V-rob and pyrollo were added to the team last year, but the number of PRs is still mostly unchanged.
I know, and I feel kind of bad about this. I've said this before, but I work better if I have a set goal instead of a vague idea. I'd probably review more PRs if I had a list of ones I should review. In concrete terms, I review PRs that have been requested of me (for instance, with the "request review" button on GitHub, on IRC, or with a PM). So, if there's a PR I can/should review, I'd appreciate it if people simply asked.
Hugues Ross wrote:
Fri Apr 02, 2021 21:53
Speaking personally, I would be interested in a position like this
Honestly, if you think that you'd like to be a reviewer without the responsibilities of a core dev, I'd suggest you find c55 on IRC and ask him if you could. If he says yes, then you're in. That's probably the best way to get this started: by actually getting someone doing it.
Core Developer | My Best Mods: Bridger - Slats - Stained Glass

User avatar
philipbenr
Member
Posts: 1897
Joined: Fri Jun 14, 2013 01:56
GitHub: philipbenr
IRC: philipbenr
In-game: robinspi
Location: United States

Re: We need more official PR reviewers

by philipbenr » Post

I've never done any development for MT but I thought I'd throw my 2 dev-cents in the ring
rubenwardy wrote:
Fri Apr 02, 2021 22:22
There is also the possibility of people being given review rights over certain areas, like a more specialised core dev. For example, we previously had someone who had the ability to approve and merge documentation PRs
Having someone that is fairly familiar with enough of the Minetest codebase will be a significant barrier to entry. This seems like a more reasonable solution, but you will still need people who possess a high enough understanding of the engine to make sure things don't diverge and become a mess (assuming that's not happening already...)
sfan5 wrote:
Sat Apr 03, 2021 13:53
What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
Documentation and roadmaps are lovely :)
v-rob wrote:
Sat Apr 03, 2021 19:56
I know, and I feel kind of bad about this. I've said this before, but I work better if I have a set goal instead of a vague idea. I'd probably review more PRs if I had a list of ones I should review. In concrete terms, I review PRs that have been requested of me (for instance, with the "request review" button on GitHub, on IRC, or with a PM). So, if there's a PR I can/should review, I'd appreciate it if people simply asked.
Based on what you said above, I suggest you look into agile methodologies, specifically Kanban boards, if you haven't already. GitHub has built-in "project" support, which I've used to create and manage my projects using Kanban boards. It will help you organize better while letting other people see what you're working on (they can be public or private). If you invite other people to join your board as collaborators, they should be able easily add things to your backlog / todolist.

---

In general, I like seeing the developer roadmaps in the forum sections, but if I'm being real, the best place is on GitHub and within the project itself. To put it bluntly, there are less children, and more people that care and are going to put the time in to understanding what exactly is going on. Not to say things are perfect (I've seen some of the GitHub issues and PR's) but it is a better platform.

Personally, I decided to be less invested in development (I just don't have the time) and be more invested in the community and philosophy of the project, as that is easier to segment into small chunks of time as needed. That's what the forums should be for imo. But for people that care about specific development, GitHub is the place to be.

User avatar
Wuzzy
Member
Posts: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

Re: We need more official PR reviewers

by Wuzzy » Post

I have just looked at the the PRs that got closed (merged or not) in the recent months (from ca. February until now). I noticed something interesting:

The ONLY approvers in the last months were sfan5 and Krock/SmallJoker and nerzhul. Nerzhul approved only rarely, the bulk approvals came from sfan5 and Krock/SmallJoker. rubenwardy made a few justified rejections of bad PRs, but never approved anything.

Note: I'm not judging anyone here, I'm just analyzing the situation.

In other words, currently, the entire PR approval process mostly rests on the shoulders of only two people right now. If one of them goes missing, the PR pipeline comes to a screeching halt because you usually need 2 approvals to get anything merged.

I didn't realize we only have 2-3 active approvers right now. This explains so much now.
sfan5 wrote:What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
Oh, this is actually a very good point. I feel like the roadmap should be given top priority. Everything else depends on it. This makes complete sense, I feel like many PRs are left open forever simply because nobody has the guts to close vaguely low-quality PRs because someone MIGHT like them. ;-)

User avatar
v-rob
Developer
Posts: 970
Joined: Thu Mar 24, 2016 03:19
GitHub: v-rob
IRC: v-rob
Location: Right behind you.

Re: We need more official PR reviewers

by v-rob » Post

AFAIK, rubenwardy is taking something of a break.
Wuzzy wrote:
Thu Apr 08, 2021 01:14
This makes complete sense, I feel like many PRs are left open forever simply because nobody has the guts to close vaguely low-quality PRs because someone MIGHT like them. ;-)
Paramat was very good at this, even though it earned him a lot of flack. It's really too bad he disappeared without a trace; it would have been nice to know why he left. Maybe all the hate added up or something. According to the forums, he last visited one month ago, although I haven't seen him say anything at all for multiple months.
Core Developer | My Best Mods: Bridger - Slats - Stained Glass

User avatar
Wuzzy
Member
Posts: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

Re: We need more official PR reviewers

by Wuzzy » Post

I think that closing bad or off-topic PRs is also a very important of the review process. It's just that it is not clear at all when a PR is off-topic in the first place.
Having a clear roadmap will definitely make it easier (psychologically) to close bad or off-topic PRs with peace-of-mind.

Termos
Member
Posts: 417
Joined: Sun Dec 16, 2018 12:50

Re: We need more official PR reviewers

by Termos » Post

Better than having reviewers is having developers who develop, know and understand the code. Having one to rule them all is better still.

That the majority of development should happen as contributions by random people isn't a great idea. Such contributions tend to suffer tunnel vision, focusing only on one isolated aspect without much regard for the big picture. Also they tend to add complexity where it could be avoided. The code rots and grows unnecessarily, bugs come and go, the developers understand less and less of it, they get more reluctant to merge PRs because "it's risky" because they can't tell any better, some beneficial PRs get starved to death, some crap PRs get merged anyways. The code gets even worse, rinse, repeat.

MT revewers don't grow on trees, the ability to do meaningful reviews comes from experience developing MT and understanding the code. Even if there were such people on the team, their time would be better spent coding rather than reviewing other's PRs, which is a PITA and no one can be blamed for not being enthusiastic about it.

deleted c0a803ab

Re: We need more official PR reviewers

by deleted c0a803ab » Post

Termos wrote:
Sun Apr 11, 2021 13:46
Better than having reviewers is having developers who develop, know and understand the code.
The less time developers have to spend to review PRs, the more time they have to spend on developing minetest.
;)

DOOM_possum
Member
Posts: 172
Joined: Sat Mar 27, 2021 22:06
In-game: DOOM_possum

Re: We need more official PR reviewers

by DOOM_possum » Post

well, the Game is Great, and suitable for anyone, the development test, has incredible new additions like "mossy cobblestone", but seems no more beyond That, like advanced World Generation, after a player has set foot into an Old Biome, or updates using a new TOOL, it also does not seem to have backup in the case of an emergency, like if a player decides to rip out a MOD and there is nothing in It's place, like "NPC" or "Villager"

User avatar
rubenwardy
Moderator
Posts: 6972
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy
Location: Bristol, United Kingdom
Contact:

Re: We need more official PR reviewers

by rubenwardy » Post

The name for an official PR reviewer is "core developer". Having official PR reviewers that aren't core developers doesn't really make sense, unless they're limited to an area like documentation. In order to review code, you need to know how it works which is why having experience with the code is important.

The only differences between a core developer and a normal contributor is that core developers can approve/reject PRs and issues, and perform releases. Reviewing each others' and contributor's code is one of the main responsibilities, if not the. They tend to be the most active contributors because they were active contributors before, and because they can self-approve.

Minetest doesn't need more code, it needs higher quality code. There are many issues with architecture, correctness, and performance that need to be resolved. Adding reviewers that don't know what they're doing won't help with that

The solution to the bottleneck is to adopt a roadmap and to better prioritise time
Renewed Tab (my browser add-on) | Donate | Mods | Minetest Modding Book

Hello profile reader

Post Reply

Who is online

Users browsing this forum: Google [Bot], TenPlus1 and 14 guests