Follow

I'm searching for resources about code review processes. Feel free to reply with your experiences too. I'm not interested in technical matters right now like how to automate parts of code review or your Opinion about the right size of a pull request. I'm asking about the social process of deciding when to merge. How do you ensure quality and keep development moving ahead without getting dragged down by disagreements? Boosts appreciated.

I'm not interested in answers that involve "this one person is in charge and makes the final call".

An interesting idea someone told me about during the Linux App Summit arts BOF was +2/-1. IIUC that means as soon as two people review a change, it gets merged, unless someone objects. That seems like a good way to keep development moving. But that leaves open the question of how to handle impasses where neither side is willing to compromise.

I'm thinking impasses should be resolved by a binding vote once the arguments for all sides have been presented and the discussion starts to go in circles. What decides the vote? Plurality? Should it be a simple one-person-one-vote system? Weighted voting for multiple options? Maybe it depends on the situation?

Also I think there should be a specific time limit that someone can block a merge because they want to review it but have not done so yet. For example, maybe two or four weeks.

I have an idea for encouraging new contributors to do code reviews. One of the two required approvals could come from anyone and one would have to come from someone with commit access.

@be How important is it to get stuff done quickly? If you can leave a dispute long enough for the scope to change / a better solution to be discovered, you might not need to compromise.

@wizzwizz4 Getting things done very quickly isn't too important. But not having releases for over 2 years is unacceptable.

@humanetech @be Yes! Participants need an account on the same forge to have a debate on the merits of a particular contribution.

I've seen situations where a review is stuck or delayed. There is a very wide range of reasons why this happens (most frequent first):

a) reviewers take very long to review
b) contributors do not address comments
c) reviewers do not read the contribution

Real disagreement is rare. And I'm yet to see an instance where it could be resolved with a vote of some kind.

@dachary @humanetech Real disagreement is not very rare in my experience.

@be @humanetech It is definitely enough examples, absolutely. You are exceptionally patient and persistent, this is most impressive.

@dachary @humanetech It's bonkers. How did I put up with this shit for years?

@dachary @humanetech @alcinnz @marie_joseph I'd sure appreciate if people outside the situation read through some of those linked pull requests gave some suggestions how to improve the process.

@dachary @humanetech @alcinnz @marie_joseph Keep in mind for some of those there was accompanying discussion on mixxx.zulipchat.com apart from the PR.

@dachary @humanetech See how 6 people immediately agreed on how the feature should work yet one person delayed everyone else for WEEKS until being overruled by a vote. github.com/mixxxdj/mixxx/pull/

@dachary @humanetech This is absurd that one person is allowed to obstruct everyone else.

@be The method I'm used to is "NOTUC" for "No Objections To Unanymous Consent?".

Someone might want it and noone must strongly oppose for a decision to pass. Yes this might slow things down, and allows stubborn people to have their way.

But as long as everyone believes in the project and wants it to progress I find it works quite well!

@alcinnz I don't think a fork is warranted. But there is one specific developer who repeatedly won't budge even when literally nobody else agrees so we need some formal process to move past these situations.

@alcinnz I've gotten into months longs circular arguments that did nothing but burn me out.

@be @alcinnz but in this case, the only thing that's needed is a majority vote.

Apache used to tally +1/0/-1 comments, and >0 after some time (like a week or something) got merged.

If people who voted -1 are unhappy, they're can create PRs to improve things and submit those to a vote.

@be @alcinnz I've also seen individuals block everything, and it's almost always the case that these individuals do not fit the team. The only long term fix for them is to leave, unfortunately.

@jens @alcinnz This person doesn't block *every*thing, but when there is a disagreement it is almost always the same person disagreeing with everybody else.

@be @alcinnz I'm not suggesting to get rid of them.

But I have found that when measures are taken to counter such problem situations, often enough the people who caused them notice the change in environment and leave.

In the best case, of course, they adapt and stay!

@jens @alcinnz Whenever I've tried to discuss the issue before there has been denial that there's even a problem...

@be @jens @alcinnz

I'm recognising these problems from the local hackspace.

"The easiest way to solve the problems is to get rid of all the people!" -quote form IRC

@alcinnz That's generally how Mixxx has operated and my experience is that trying to achieve unanimous consent devolves into weeks/months of circular arguments.

@be I think outside review should be encouraged indeed. It may also be a way to _get started_ with contributing!
People who aren't comfortable with the codebase can learn it faster that way.
Does any forge in existence allow outsiders to do a _formal_ review, rather than just leave a comment?

@dmbaturin It's hard to get people to overcome imposter syndrome with code reviews...

@be I'm only talking about the breadth of codebase knowledge, not possible psychological issues.

@dmbaturin Almost every time I ask new people to do code reviews they've said they don't know what they're doing.

@be Now that you say that, I think projects can benefit from code review checklists!
E.g. "I have tested it and it works for me with this data:", "This is a scenario that breaks this code", "I believe this may cause this problem in the future" etc.

@dmbaturin Most often what we really need doesn't even require coding skills: "This feature makes sense and I agree it will be a good improvement."

@be I guess you are right. I've only seen and done code review in high-responsibility projects that require professional reviewers to prevent bad things from happening, but many more projects could benefit from code review—it's just not critical enough for them to bear the costs requiring it.

@dmbaturin We need more than "I tested this and it works as described." We need "I tested this and I like the change it implements and it works as described."

@be Perhaps these should be three separate points though. :)

@be Let's actually make a reusable checklist! I made a quick draft under CC0, available for contributions on Codeberg: codeberg.org/dmbaturin/code-re and Microsoft GitHub: github.com/dmbaturin/code-revi

@be As much as I want to support alternatives, I'm realistic about the numbers of people who will register there just to contribute to it... it will take a long time before not being on GitHub will mean not sacrificing anything I'm afraid.

@dmbaturin What would you sacrifice by not being on GitHub?

@dmbaturin People who won't contribute to something because it isn't on GitHub probably aren't worth collaborating with.

@be I wouldn't take such a stance. Contribution effort threshold is a real thing. When you already have an account and are familiar with the process, it's much lower.
A large, popular, and important project can tell people to sign up for a new forge account, a listicle definitely cannot.

@dmbaturin IMO people who can't be bothered to make an account aren't worth collaborating with.

@be @dmbaturin dunno about other projects, but a lot of the pull requests on Freedoom are posted on behalf of other people who aren't on GitHub.

and there was another project last month where i added a small thing but didn't want to use my GH account so fortunately they included an explicit option to email them

Show newer
Sign in to participate in the conversation
Fosstodon

Fosstodon is an English speaking Mastodon instance that is open to anyone who is interested in technology; particularly free & open source software.