About pull request reviews

I think the critical point is expectations about a contributor’s involvement into finishing up a pull request.

There are different mindsets about that. Some contributor’s intention might just be to get it done somehow. After doing the heavy lifting, they might not be interested in addressing change requests in a (maybe lengthy) review process. They’d rather be comfortable with someone else to take over the integration process and drive it home. Reasons for this could be lack of experience or time, for example.

Other contributors however are more attached to their changes and intend to manage a PR until it get’s merged. They are familiar with the code and it’s their product, so there’s objective and subjective attachment. And they’d rather implement requested changes themselves than have someone else step in. This can be very productive and it’s an opportunity to grow and evolve, both for the individual as well as the entire project. This is common for experienced contributors and those who want to become more involved.

Of course, there are many different aspects in between. For example, substantial changes might be considered different than trivial fixes. The time frame is also a factor.

From a maintainer’s perspective, I prefer authors sticking with their PRs. In my experience it’s more effective and has lasting effects. And that’s what I would expect as a default.
But it is be totally fine if an author want’s to hand off a PR for someone else to take over. We need to be more encouraging to do that. It doesn’t do any good if authors get frustrated during the review process and would be happy if maintainers just took over to mend it the way they want it to be.

At the core, this is a communication problem. We need to establish an understandig about the extend of engagement in a PR’s review process.
Maybe a simple comment would help to clarify that:

If at any point you don’t feel comfortable editing this PR, feel free to say so. Someone else will take over the process of getting it ready to merge.

This could be a standard text to use whenever a reviewer leaves a comment asking for changes.

3 Likes

Regarding the process of taking over a PR, there are different approaches. Changes can be made directly on the PR which is especially useful for simple changes. Only maintainers can do this, though. Non-maintainers can suggest changes in the GitHub UI, which works well for very simple changes.
For more complex changes, it might make sense to open a new PR. You don’t need to be a maintainer to do that. It’s a fresh start and get’s rid of old discussions about code that might’ve changed in the mean time anyways (of course, there should be a reference to the original).

1 Like

They get attached to a milestone after awhile…it’s fairly confusing, it feels like your PR is just in limbo until then…

In general it might be nice for PR’s to be given a target, maybe somebody could add a comment at the bottom “we’re still waiting on X for this PR” so people know what is going on for “stalled” PR’s?

1 Like

Possibly “pending_milestone” or “consider_for_next_release” tag?

1 Like