Reviewing PRs

Development
1

This is a Topic for any discussion around the process of reviewing PRs (not discussing specific PRs), for the purposes of transparency, onboarding, and because a number have questions have come up on the topic in different places recently.

The wiki page on Reviewing offers some basic information, but a lot of knowledge is held by Reviewers themselves, so hopefully we can make this a place to share knowledge. (I’ll do what I can to migrate information from here to the wiki.)

Some related wiki pages:

1 Like
2

I’ll start…

I have Reviewer status, though I typically limit my contribution to discussions, with an occasional “proper” Review and Approval. I’m considering getting more involved because we’re approaching triple digits on the number of outstanding PRs. I made my first Merge yesterday on a minor PR, to see what the process was, and considering I may be able to help bring the number of approved but unmerged PRs down.

But I’ve had some lingering questions…

  1. It would be helpful to know if there is any understanding of who can perform merges. I have always assumed it was contributors with “Trusted Reviewer” status. But I see that list is very short and we obviously have more people making merges than that.
  2. Is there a process to perform between a PR being Approved and it being Merged? For example, a grace period for other interested people to have eyes on it that may have been away during the review.
  3. Is there any particular communication with the Release Manager before/while performing merges?
  4. Can someone relay their experience with performing Merges, any preparatory work/organizing, things to look out for, common mistakes, etc.

Also, a rough hand count of people currently willing to perform both Reviews and Merges would be helpful.

I should mention this comes after a long line of comments on the stagnant state of moving PRs over the finish line, which has been discouraging for some (one example on this forum). There’s been an encouraging uptick in discussion on PR threads though. :slight_smile:

One concern is that some of these approved PRs should spend some time in circulation in the dev build for testing, rather than waiting for a release push when a couple dozen changes will flood the develop branch.

4 Likes
3

@jamshark70 , I’ve had a similar idea recently. We could modify the template so that under the “Ready for review” check box we instruct something like:

“If this PR is not ready for review, briefly describe the kind of feedback you’re looking for by posting this PR”

We might also make use of labels, create one that is like “author requests feedback”. The closest one currently is “question”, which doesn’t seem to fit.

1 Like
4

I think those cases are very often just ignored. The author can make a change, and sometimes it’s not polemic, not even a big change, but it’s an enhancement nevertheless. He doesn’t know how the e community (or the leadership, actually) will receive it. For example a small change in the C++ code that makes a function safer, or some optimization, or anything. After one week, or two, one just gives up.

Sometimes the author has a bigger idea, it’s good and makes sense, but just needs one more pair of eyes to keep going forward. This also happens a lot. And sometimes I see the PR being closed.

5

So combining your suggestions, perhaps the instruction “if not ready for review please label this PR with the “author requests feedback” label and briefly describe the feedback you are looking for below” ?

6

Yep, that’s the thought offhand… if we don’t hear from folks that have done a lot of review triage to know if this would actually be helpful, I guess we could try it out and see if it gets used.

1 Like
7

I think it depends very much on what is the change.

I keep changes for myself because sometimes I just don’t want the trouble.

When it’s uncontroversial or bugfix, it’s easy. Although there are excellent reviews right now. But in the past, I had not-so-good experiences.

Example: I had a PR with more than 100 messages in which one dev was making every excuse to create problems, it was not rational, and others thought it was good.

Recently I was trying to optimize some code and I tried another gcd algorithm (which I would never propose, because no way) and I found a minor problem in the codebase. The code could be much safer by changing some things to prevent overflow. It was small and uncontroversial, but I knew it would probably be ignored or people just wouldn’t change some parts of the codebase.

8

The main idea is to coax out a bit more info from the author as to what their intention is, if not to have code reviewed and considered for merge.

E.g. “I’m stuck in this implementation detail…”, “there are multiple solutions, let me know what you think of this one”, etc.

Sometimes authors just breeze through the description and it’s not until a conversation is underway that you learn they actually don’t consider the work complete. Meanwhile the reviewer just sank into the project intending to chip away at the many PRs that ARE ready for review. Momentum lost…

9

Yes, those things could be more clear as well.

10

Yes this is unfortunate and I’m not sure what to do in these situations. Not knowing the particular case, I’d imagine at some point the conversation becomes hard to track at that length.

It may be worth steeping back from that thread, even closing the PR, distilling both the approach and the concerns down to a concise summary, and try again in another forum if consensus was the problem, or a fresh PR that summarizes progress and acknowledges concerns. Reset the conversation…

I know if I happened into a thread that long, I’d likely turn right around and head back out :sweat_smile:

1 Like
11

At the same time, I touched recently some parts of the scheduler, and I had very good support and it was a good experience. Something is changing.

Some people have some theories about some people can’t touch certain things, maybe. I don’t know. It’s not objective.

1 Like
12

Anyway, maybe all this has something to do with me using another language and writing my system. I found another community that is very serious and very friendly at the same time. I learned a lot.

13

Hi all,

Thanks for the discussion above. My thoughts:

On PRs that aren’t quite ready

Just to check, we know that GitHub has draft PRs? If it’s not ready, but feedback would be helpful, then I’d recommend using that feature.

On PRs

A PR is a proposal to incorporate a set of changes into the codebase. As a result, the conversation on a PR should be about what needs to change about the proposal, if anything, in order to merge the PR. Discussions need to be highly focused on the code at hand, and heavily moderated to stay on topic. So while I agree that there has been lots of discussion on some PRs recently, some of those discussions should be carried out elsewhere - likely using GitHub Discussions. A key thing is knowing when a discussion needs to take place outside of the PR context - a good indicator being if we’re not talking about the code in the review anymore.

PRs take a lot of work to write. And they take a lot of work to review. There has to be patience and understanding on both sides of the equation.

Some general advice for PRs:

  • PRs must be as small as possible. Only one thing should change. It’s less to write, and less to review!
  • PRs must have appropriate descriptions, and not just say “solves issue 236”.
  • PRs must be tested. Without tests, we don’t know if it works, and we won’t know if other changes later on break it. Ideally, we should be able to read the tests as documentation, and if they pass then it’s very easy to accept.
  • Pass CI. SuperCollider is a cross platform project, we can’t accept things that work on one platform but not another. Without CI, reviewers can’t be sure that the code is safe to merge into the project.
  • Participants in the PR must follow the code of conduct
  • Comments should be directed at the code/Project, not people
  • Asking for changes means asking for more work. That doesn’t mean we should accept everything, but things like personal opinion shouldn’t be enough of a reason to request a change: always try to provide additional information where necessary to back up assertions and justify the change-request.
  • Reviewers are doing their best and it might take a while to get around to it
  • Submitters are doing their best and it might take a while to get around to it
  • Take discussions away from the PR if necessary
  • Code reviews don’t catch bugs: tests do

On procedure

SuperCollider uses the git-flow procedure - which has its issues, but that’s another topic - to ensure that we have effectively two versions:

  • Dev: bleeding edge, stuff might be broken, please use it at your own risk and let us know if there are any issues.
  • Release: stuff that has been run in Dev long enough that we’re confident it is stable.

This helps remove some pressure on getting the review right the first time - but we still need to aim for good code to begin with.

We have a few other things in place to have safer merges:

  • You can’t merge your own PR: no-one can bypass the review process
  • It needs to be reviewed by someone with write access: trusted reviewers have write access to merge code
  • It should ideally be reviewed by one person, and merged by another
  • It needs to pass CI

Currently, some of this might not be configured correctly, please let me know if something doesn’t look right.

I’d like to set up some teams on GitHub that are responsible for certain parts of the codebase, and require reviews from someone in those teams - but I don’t feel I have the authority to do that on my own.

If GitHub Discussions aren’t set up, and we’d like to use it, please let me know and I can sort that.

On next steps

  1. I’m happy to write a guide for conducting reviews if it’d be useful, but there’s lots of good information already on the wiki.
  2. We need to fix CI - in my view, all other reviews are questionable until then - nothing should be merged until it’s running again. It should be the main focus of the project at the minute.

“But James, why aren’t you reviewing things?”, I hear you cry.

Apart from the CI issues, it takes a lot of energy. I’m trying. I don’t feel like I have enough authority to make the decisions an approval would require. This is one of the issues with the do-ocracy, but that’s another topic.

We used to do swarm reviews where we’d have a call and review things together. I think those worked well and helped a lot - and can get people used to what it is, so they are very good for bringing people along.

Apologies for another “long James post”, but I really care about this stuff.

Best,
J

4 Likes
14

I’m afraid to say that but writing a few UnitTests and saying it “catches bugs” is such an exaggeration.

Unfortunately, certain beliefes are very rooted in this community, and this is one of them.

One of the things this project needs the most is to take tests more seriously, because the codebase is aging, and this problem will aggravate by the day.

I tried to introduce a discussion about it and around property-based tests, and it was clear the community has no idea or just thinks it does not matter.

But I have to say I’m very glad those things are being discussed. Something needs to be done regarding pr and review.

15

There is a new contributor with more experience with CI, and it should be the right time to discuss what to expect from it.

16

I’m quite sure this is not what James meant. It’s of course correct to label this stated position as naive, but I don’t think that’s relevant to the post in question.

A problem that I have had with unit testing is that, often, it may take 5-10 minutes to write a code fix (longer to diagnose, of course), but anywhere from 3-5 times longer to write a meaningful unit test (and then I had code reviews in which I was expected to take a lot of time to work around weaknesses in the UnitTest framework). This is one of the main reasons why I largely quit contributing. I agree that tests are necessary, but does it have to be that painful?

hjh

17

One of the major problems is that even with simple tests, the CI takes more than 20 minutes, and this situation in general (I think) is influencing a lot the way the community sees tests.

Tests are hard. And a Unit Test written by the same person who wrote the code (with the same focus and frame of mind) is too little. And we need to develop the aspect still.

18

Another thing, in my perspective, in a dynamic system like SC, an isolated Unit Test is taking a bath before you get dirty. Because of its nature, tests would also need to check the system interacting in different ways, in different forms.

To achieve this, the properties of the system and parts of the system need to be carefully defined. After that, things get easier.

19

I apologize if that sounded weird. I don’t know what people think because nobody wants to talk about it. Maybe there is a history that I’m not aware of.

20

Great post, @jrsurge, we’re already putting some ideas into practice now.