Matthias, Your suggestion seems workable too. PRs are merged with outstanding change requests indicated — a reviewer comments, the comments are addressed then a different reviewer approves without the original review being removed. The labels are a bit more in your face. A hybrid “hold: required changes see review/comments” might be workable.
Pauli -- Dr Paul Dale | Distinguished Architect | Cryptographic Foundations Phone +61 7 3031 7217 Oracle Australia > On 10 Sep 2020, at 4:03 pm, Dr. Matthias St. Pierre > <matthias.st.pie...@ncp-e.com> wrote: > >> Just wondering if we should have two new labels: “hold: tests needed” and >> “hold: documentation needed” labels? >> There are a number of PRs that come through where one or both of these are >> missing missing. > > The two use cases you mention are actually better handled by a change request > (via GitHub review) instead of a label: > A team member can formulate a change request to block the merging. Here he > has more freedom to formulate what > needs to be done. This includes your two use cases, as well as the use case > for the label [hold: need rebase]. > > The problem with it is that currently we count only approvals and don’t > consider unresolved change requests > as a blocker. I think we should change that. This does not mean that a > reviewer who made a change request > two months ago and lost interest is forced to re-review, only that such stale > reviews must be dismissed > explicitly, if the reviewer does not respond to a re-review request within a > certain time period. > > Matthias >
