In TFS, when we put comments for things to be fixed in a Pull Request before we will accept it, should we mark it as Reject or Wait For Author? Which is better?
TFS – Should Reject or Wait For Author be Used When Fixes are Needed?
pull-requeststeam-foundation-server
Related Solutions
Accepting pull requests that are slightly behind master seems perfectly fine to me, unless there's a strong reason to believe that conflicts are likely.
Ideally most of your pull requests should be on different parts of the codebase, and those parts should be decoupled enough that any one of them is very unlikely to break most of the others. In this case, forcing all but one of the requesters to update their pull requests is little more than red tape.
Also, in many cases it should be possible for you to verify things work when you accept these requests, because 1) you have automated tests (right?) and 2) pull requests should come with a minimal code example that is either fixed or enabled by that pull request, or at least a detailed enough description to produce such an example.
But what if you have several pull requests that clearly will conflict with each other? If they're all trying to do the same thing, pick the best one and ask the other requesters to test it to make sure it fixes the problem for everyone. If they're all trying to do different things to the same piece of code, and there's no sane way to detangle them (this should be very rare!), then I would bite the bullet and try doing some git surgery to combine them into a single pull request that you can ask everyone to test before accepting.
From what I've seen, most of those steps are done on Github by convention, and not by any official Github-provided process.
My employer uses Github, I run a good number of small open source projects, and make occasional contributions to other open source projects.
Here is how I've usually seen it done:
Author adding his/her colleagues as reviewers:
This varies from project to project, but in general, the assigned peer reviewers are all of the contributors to the project.
Open source projects seem to have a rough hierarchy - maybe their convention would be to only merge after a "core" contributor has given the okay.
At the shop where I am currently employed, we merge after any one of the half-dozen developers on the team has given their approval.
On rare occasions someone on the team may use a comment to specifically call out another developer that they think should peer review the code before it is merged, but otherwise, whoever gets there first and feels like doing so can review and make comments.
Reviewer approval:
Approval is typically shown by making a comment on the pull request saying "+1" or "lgtm" (looks good to me).
Lightweight tasks:
I've used the checkboxes as well, but in most cases, every comment on a pull request is considered an implicit "task" that is resolved either by:
- changing the code that the line is commenting on
- responding with another comment
Seeing at a glance what is approved and what still needs reviewed:
I have used the Looks Good To Me extension for Chrome, which gives you such a view from the Pull Requests screen. The pull requests list view seems to have been broken by recent Github changes, though.
Best Answer
As per Microsoft on Review code with pull requests: Vote on changes the suggested purpose of each class of approval is:
Thus I take
Waiting for Author
to mean that the you think the Author screwed up in his/her approach to the solution but that his/her code is redeemable if they take your comments to heart.And
Rejected
means that no way in hell are you accepting any change like this no matter how well written the code is.The question you have to ask yourself is whether your groups idea of the suggested purpose matches Microsoft's idea.