<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Matthias,<div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Pauli<br class=""><div class="">
<div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">-- <br class="">Dr Paul Dale | Distinguished Architect | Cryptographic Foundations <br class="">Phone +61 7 3031 7217<br class="">Oracle Australia</div><div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><br class=""></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline">
</div>
<div><br class=""><blockquote type="cite" class=""><div class="">On 10 Sep 2020, at 4:03 pm, Dr. Matthias St. Pierre <<a href="mailto:Matthias.St.Pierre@ncp-e.com" class="">Matthias.St.Pierre@ncp-e.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><blockquote type="cite" class="">Just wondering if we should have two new labels: “hold: tests needed” and “hold: documentation needed” labels?<br class="">There are a number of PRs that come through where one or both of these are missing missing.<br class=""></blockquote><br class="">The two use cases you mention are actually better handled by a change request (via GitHub review) instead of a label:<br class="">A team member can formulate a change request to block the merging. Here he has more freedom to formulate what<br class="">needs to be done. This includes your two use cases, as well as the use case for the label [hold: need rebase].<br class=""><br class="">The problem with it is that currently we count only approvals and don’t consider unresolved change requests<br class="">as a blocker. I think we should change that. This does not mean that a reviewer who made a change request<br class="">two months ago and lost interest is forced to re-review, only that such stale reviews must be dismissed<br class="">explicitly, if the reviewer does not respond to a re-review request within a certain time period.<br class=""><br class="">Matthias<br class=""><br class=""></div></div></blockquote></div><br class=""></div></body></html>