Avoid re-requesting the same reviewer

[turion]

See this PR: NixOS/nixpkgs#91662

The bot has added and removed the same labels a few times. Maybe that was just for debugging purposes, or maybe it's a bug.

[timokau]

Unfortunately the logs don't go far enough back, but I suspect that the bot actually requested a review of @ryantm 3 times and github only shows the first one. So the transition is:

• Notice that the reviewer is not responding, set to needs_reviewer.
• Triage needs_reviewer PRs, yours is probably the oldest so it gets first dibs on reviewers.
• Assign the only available reviewer, r-ryantm.

I eventually want it to post a warning/reminder before timing out the "needs reviewer". That would make the process a bit more obvious and then it might be a good idea to avoid re-requesting the same reviewer twice. Marvin is currently on my backburner though, so it might take a bit until I get to that.

[timokau]

I just discovered in marvins logs that I had been re-requested for #91570. I didn't get an email. I have implemented the reminder by now, which somewhat alleviates the situation. Still, it would be better to avoid re-requesting the same reviewer (or at least do it with an @mention instead) since it won't generate any notification. I'm not sure if its possible to determine that though. As a proxy, we could check who has been involved in the PR at some point.

[asymmetric]

@timokau another case of this happened here: I was chosen as a reviewer, but after building I decided I wanted another pair of eyes on the PR, so I triggered needs_reviewer. Marvin then proceeded to add me again as a reviewer.

I wonder if it could instead pick someone else. Or maybe I'm not using the flow correctly, and should've added needs_merger instead - but I didn't approve the PR, that's why I didn't do that.

[timokau]

I think a good way to handle the "I don't feel qualified" case is to try to identify somebody who has more experience in that area and request a review from them. The reviewer's responsibility is to move the PR forward in some way. Most of the time they can do that by reviewing it, sometimes they can choose to delegate instead. "Shepherd" is maybe a better term for the concept.

You already did that in NixOS/nixpkgs#111796. In that case I would suggest to just leave the awaiting_reviewer label. Next time it may be better to ping fewer people at a time though. That might actually increase the probability of a response (diffusion of responsibility with many possible reviewers) and avoids unnecessary notifications.

All in all I think you responded well in that MR. Thanks!

[turion]

"Shepherd" is maybe a better term for the concept.

That, or maybe it's closer to the role of an assignee than a reviewer. That role even exists in Github nowadays. Maybe it makes sense that marvin sets the assignee of a PR?

[timokau]

Yes, I agree. The switch would also have other benefits. I'm not sure if we should also change the label names in that case.