A guide to reviewing PRs for merge

[I-am-Erk]

Summary

Infrastructure "A guide to reviewing PRs for smoother and more consistent merging"

Purpose of change

Per the doc itself:

As the CDDA project grows, it is becoming harder for the few volunteers in project management to keep constant tabs on what gets merged. Over the years, we've found that there are two possible ways this shakes out. Whenever the lead devs are away for real-life reasons for a while, either we get a huge backlog of PRs, or we wind up with a few things getting merged that really shouldn't have been. This leads to having to revert work by enthusiastic and well-meaning contributors, which we consider an very bad outcome.

Describe the solution

This document aims to do two things to reduce this:

1. Develop a standardized PR review process and checklist that can be used for complex PRs, and
2. Through the above, offer an easier way to assess if a PR is ready to be merged.

This is not required for all PRs, and we don't expect that to change. Rather, we hope that a standardized checklist would help contributors and collaborators know what to do to review and approve a complex PR, which in turn would make it much easier for people with merge permissions to go through and assess for merging. Additionally, this checklist could be used as a guide for people making PRs to see if they think they're ready to go. Please don't use this as an excuse to 'rules lawyer' the project; a PR could potentially break all of these points and still be mergeable. These are guidelines.

Describe alternatives you've considered

This isn't strictly necessary. The most important step to preventing the reversions we've seen lately is simply to make sure our people with merge permissions have been more thoroughly trained by the senior devs. However, this has happened twice now, and once was too much. This is another good stage to try to standardize things.

I also really hope that this will help encourage people to be more proactive about reviewing PRs, as that's one of the best overall ways to get the project running more smoothly and to encourage merging.

Testing

n/a

Additional context

This is a living document, feedback on wording and content is welcome.

[Inglonias]

It would be good if a link to this document were automatically included in our PR process somewhere visible - A guide is useless if nobody reads it.

[Inglonias]

If you'll forgive the double post, the line count is easy enough to automate - I would suggest we have a bot count up the number of lines in a PR, and if they exceed the threshold, pop a link to this document in there with a note that this is a PR that needs more careful review.

[I-am-Erk]

It would be good if a link to this document were automatically included in our PR process somewhere visible - A guide is useless if nobody reads it.

I agree that it should be linked somewhere, but the PR blurb itself is already too long and mostly unread. The idea of having a bot that can detect certain common spots it's needed is a good start. A discord shortcut like we have for -fms is also a good idea.

[Inglonias]

I'm not talking about the PR blurb. I'm talking about automatically leaving a comment on the PR a la our spell checker.

And after thinking over our discord conversation, if you want to require more careful review by senior developers for "sufficiently large" PRs before they are merged, I wouldn't be against that.

[I-am-Erk]

I think we might be able to do it by checking (1) size (ideally distinguishing c++ from json) and (2) number of labels. The more labels a PR has the more likely it is to require review, particularly now that we have an automatic labelling system.

[oosyrag]

I for one, as a newbie, did not know "reviewing" was a thing that anyone could do and thought it was for maintainers/senior devs/bots. So this is nice. Although a link from somewhere relevant would definitely be helpful. There are still docs that I haven't explored or read in the docs directory, two years later lol.

[Knut-Aage-Hofseth]

I wholeheartedly approve of increased transparency and think this is a good step forward.

I have only contributed a couple of lines of JSON, but I read PRs to keep up with experimental changes. More relevant is the fact that I work as a software engineer that has to conform to Company Best Practices for development cycles.

Clear communication and clear expectations are a good thing. I would also wish for clearer guidelines on what feedback should be given when PRs are rejected or reverted. Good communication is key to collaborations, and no well-meaning feedback is ever as bad as the imagined feedback when all one gets is silence.

[Brambor]

I read the thing.

[I-am-Erk]

guidelines on what feedback should be given when PRs are rejected or reverted

That one can be very tricky but it's something we're looking at.

[NetSysFire]

To be honest: It feels like a rambly blog post right now and should be much more concise. Guidelines or rules are hard to read in that format.

I also strongly suggest to use more affirmative language. Specific example: this does NOT mean you should immediately close your large PR - replace it with e.g you may carry on with your PR - however, consider splitting it into parts.
You use "not" and "NOT" a lot in this document where it can be reworded to produce results that are intuitively easier to understand.

If you want to read up on the specifics, see e.g https://blog.wordvice.com/grammar-avoid-double-negatives/ or https://en.wikipedia.org/wiki/Reactance_(psychology). This is partly the classic problem of when you put up a sign onto a fragile object that says "Do not touch", which immediately tempts people to touch it because our brains are dumb (could also be related to call of the void?). Putting "Touching will damage the object" instead is much more effective.

Wording this review right now causes https://xkcd.com/1915/ in me. So I'll just finish for now and submit this review.

[I-am-Erk]

Thanks for the proofreading. I agree it's a bit verbose, I just tend to be that way; fixing it requires cognitive load that I probably should safe for other things. I'm open to someone making a more detailed editing pass in a follow up PR but I'm out of steam to do it myself on this one.

[Knut-Aage-Hofseth]

I just went through this and changed some instances calling the guidelines rules for consistency.