openrw icon indicating copy to clipboard operation
openrw copied to clipboard

Current merge system doesn't work well

Open ghost opened this issue 7 years ago • 2 comments
trafficstars

Reviewing and merging is quite absorbing task. Lately only @danhedron has been taking care about it. It's too much work even for someone with a lot of free time. So I have 2 suggestions:

  • assigning more people for reviewing and merging,
  • after two weeks without review (trusted?) collaborator is allowed to merge pull.

What do you think? @danhedron @tsjost @JayFoxRox @darkf

ghost avatar Apr 29 '18 22:04 ghost

I still review issues from time to time and engage when I'm asked, but other than that I've left OpenRW behind. I just don't have any time or motivation to work on it. I'm busy with other projects and life. However, below is my opinion / thoughts on the situation.


With volunteers, you can't just "assign more people to review" (giving them permissions is fine though). Assigning people often results in pressure and takes the fun out of things - so you'll actually loose contributors.

Instead, it is important that especially beginners understand that one of the best ways to help the project is to review other peoples stuff (both, issues and code / PRs). However, until that mindset has been established we are doomed.

We are also doomed if there isn't enough people in general: There's always a sweet spot were you have enough active coders and some of the less active coders review the code.

But:

  • Too many coders: Nobody reviews (no chance to review because too much code is written, especially small features).
  • Too few coders: Nobody reviews (no chance to review because everybody is working on features to push the project forward).

tl;dr: We currently don't have enough people = too few coders. And the coders we have, don't do enough review.


Typically what will happen in such cases, is that developers get frustrated and leave the project, resulting in project stagnation. The other scenario which could happen is that developers will try to blame this on the maintainers which will result in a hostile fork. However, the problem is not actually with the maintainers, but the community not having a right mindset about reviews. So the new hostile fork will also result in project stagnation, and also damages the original projects reputation.

tl;dr: There'll be project stagnation and it might hurt the project even more if we don't do anything.


My suggestion on IRC was what @ShFil119 already said in the first post (+ some small additions):

  • Trust the most active coders and allow them to merge their own code under the following rules:
    • Code must have been public for ~1-2 weeks and not gotten sufficient review.
    • Code needs flawless documentation in the PR description and code, so other people can figure it out, even if it has been merged months ago, with the author unreachable.
    • Before merge, there should be a post warning that the merge will happen soon (at least 24 hours before the actual merge). This way people can veto.
    • The author should be critical of their own work and review it one final time themselves before merging (and possibly delay the merge if they find mistakes).
    • During this time, avoid work on entirely new subsystems or huge features which are typically troublemakers in reviews anyway. If need-be, there should at least be some mandatory discussion with the maintainers during the design phase. If that's not possible, work on other parts of the project instead - there's many smaller things to do anyway which are just as-important.
  • Small changes should be marked for taking a quicker review path, so the maintainers can accept them without a review by others.
  • Try to improve the README and other documentation and community aspects to inform people that we need help with reviews (actively encourage new users to learn from the PRs and make sure they also understand what is being merged - they are the future of the project).

JayFoxRox avatar Apr 29 '18 23:04 JayFoxRox

Agreeing with "if nobody reviews in a reasonable time, and the author has reviewed it himself and is sufficiently satisfied that it has no major issues, merge it."

Unless @danhedron has objections, anyone interested should mention that their PR is OK to be reviewed or merged and me or one of the other people with merge rights will merge it in, I think that's fair.

darkf avatar Apr 30 '18 09:04 darkf