
<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi <div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On 01 Feb 2017, at 4:43 PM, Matthias Kuhn <<a href="mailto:matthias@opengis.ch" class="">matthias@opengis.ch</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi Tim<br class=""><br class="">On 02/01/2017 03:18 PM, Tim Sutton wrote:<br class=""><br class=""><blockquote type="cite" class="">What do others<br class="">think of my proposal to require peer review before merging PR's? I think<br class="">it will help prevent situations where code gets merged before people<br class="">have had a good chance to review it in future.<br class=""></blockquote><br class="">I don't know the tools you propose very well. Can you explain in more<br class="">detail what they will change?<br class=""><br class="">In particular, does the proposal mean that the master branch is<br class="">protected from pushing and every commit is forced to be proxied over a<br class="">pull request? That would make me a bit afraid of the overhead this will<br class="">impose on the few devs which are currently checking taking care of the<br class="">PR queue.<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div><br class=""></div><div>Basically if you look at this image:</div><div><br class=""></div><div><a href="https://cloud.githubusercontent.com/assets/178003/22511566/c9e0fb12-e89e-11e6-9d07-6ace557cb5a8.png" class="">https://cloud.githubusercontent.com/assets/178003/22511566/c9e0fb12-e89e-11e6-9d07-6ace557cb5a8.png</a></div><div><br class=""></div><div>I propose to enable the two checkboxes:</div><div><br class=""></div><div><label style="box-sizing: border-box; font-weight: 600; color: rgb(51, 51, 51); font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';" class="">Require pull request reviews before merging</label><span style="color: rgb(51, 51, 51); font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol'; background-color: rgb(255, 255, 255);" class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; font-size: 13px; color: rgb(102, 102, 102); display: block; font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';">When enabled, all commits must be made to a non-protected branch and submitted via a pull request with at least one approved review and no changes requested before it can be merged into <span style="box-sizing: border-box; font-weight: 600;" class="">master</span>.</span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; font-size: 13px; color: rgb(102, 102, 102); display: block; font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';"><br class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; font-size: 13px; color: rgb(102, 102, 102); display: block; font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';"><label style="box-sizing: border-box; font-weight: 600; font-size: 14px; color: rgb(51, 51, 51);" class="">Require status checks to pass before merging</label><span style="color: rgb(51, 51, 51); font-size: 14px; background-color: rgb(255, 255, 255);" class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;">Choose which <a href="https://developer.github.com/v3/repos/statuses/" data-ga-click="Protected Branches, go to statuses docs, from:branch settings required status checks checkbox" style="box-sizing: border-box; -webkit-text-decoration-skip: objects; color: rgb(64, 120, 192); text-decoration: none;" class="">status checks</a> must pass before branches can be merged into <span style="box-sizing: border-box; font-weight: 600;" class="">master</span>. When enabled, commits must first be pushed to another branch, then merged or pushed directly to <span style="box-sizing: border-box; font-weight: 600;" class="">master</span> after status checks have passed.</span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;"><br class=""></span><span class="">The former would require that the PR is approved by someone else and the latter would require that all tests pass. They do not as far as I know prevent direct pushes to the upstream QGIS master branch (but we should probably culturally avoid that as default behavior).<br class=""></span><span class=""><br class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;">Regards</span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;"><br class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;"><br class=""></span><span class="note" style="box-sizing: border-box; min-height: 17px; margin: 0px; display: block;">Tim</span></span></div><br class=""><blockquote type="cite" class=""><div class=""><div class="">Regards<br class="">Matthias<br class=""><br class=""><blockquote type="cite" class=""><br class="">Regards<br class=""><br class="">Tim<br class=""><br class=""><br class=""><br class="">---<br class=""><br class="">*Tim Sutton*<br class="">QGIS Project Steering Committee Chair<br class=""><a href="mailto:tim@qgis.org" class="">tim@qgis.org</a> <<a href="mailto:tim@qgis.org" class="">mailto:tim@qgis.org</a>><br class=""><br class=""><br class=""><br class=""><br class=""></blockquote>_______________________________________________<br class="">Qgis-developer mailing list<br class=""><a href="mailto:Qgis-developer@lists.osgeo.org" class="">Qgis-developer@lists.osgeo.org</a><br class="">List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer<br class="">Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer</div></div></blockquote></div><br class=""><div class="">

<span><img apple-inline="yes" id="BE991580-AE95-4198-8851-5973997B8280" src="cid:879A6E78-CA46-47B2-AA0E-1810BD833229" class=""></span><div style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; line-height: normal; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div style="font-weight: normal;" class=""><br class="Apple-interchange-newline"><br class="Apple-interchange-newline">---</div><div style="font-weight: normal;" class=""><br class=""></div><div class=""><b class="">Tim Sutton</b></div><div style="font-weight: normal;" class="">QGIS Project Steering Committee Chair</div><div style="font-weight: normal;" class=""><a href="mailto:tim@qgis.org" class="">tim@qgis.org</a></div><div style="font-weight: normal;" class=""><br class=""></div></div><br class="Apple-interchange-newline" style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; line-height: normal;"><br class="Apple-interchange-newline" style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">

</div>

<br class=""></div></body></html>