From 2f7e28d1f700e1cdafb7771d4a61d8f71b68df04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 7 Oct 2016 15:28:15 +0200 Subject: Improve the contribution and MR review guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/code_review.md | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'doc/development/code_review.md') diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 40ae55ab905..98279948888 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -34,6 +34,10 @@ request is up to one of our merge request "endbosses", denoted on the ## Having your code reviewed +Please keep in mind that code review is a process that can take multiple +iterations, and reviewer may spot things later that they may not have seen the +first time. + - The first reviewer of your code is _you_. Before you perform that first push of your shiny new branch, read through the entire diff. Does it make sense? Did you include something unrelated to the overall purpose of the changes? Did @@ -55,6 +59,7 @@ request is up to one of our merge request "endbosses", denoted on the Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then: +- Try to be thorough in your reviews to reduce the number of iterations. - Communicate which ideas you feel strongly about and those you don't. - Identify ways to simplify the code while still solving the problem. - Offer alternative implementations, but assume the author already considered @@ -66,6 +71,8 @@ experience, refactors the existing code). Then: "LGTM :thumbsup:", or "Just a couple things to address." - Avoid accepting a merge request before the build succeeds ("Merge when build succeeds" is fine). +- If you set the MR to "Merge when build succeeds", you should take over + subsequent revisions for anything that would be spotted after that. ## Credits -- cgit v1.2.1 From 52ca9bf600235459721fbcf16e4f582b839b3b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 7 Oct 2016 16:17:28 +0200 Subject: Fix typo and add he MWBS accronym for "Merge When Build Succeeds" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/code_review.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'doc/development/code_review.md') diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 98279948888..c5c23b5c0b8 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -35,7 +35,7 @@ request is up to one of our merge request "endbosses", denoted on the ## Having your code reviewed Please keep in mind that code review is a process that can take multiple -iterations, and reviewer may spot things later that they may not have seen the +iterations, and reviewers may spot things later that they may not have seen the first time. - The first reviewer of your code is _you_. Before you perform that first push @@ -69,9 +69,9 @@ experience, refactors the existing code). Then: someone else would be confused by it as well. - After a round of line notes, it can be helpful to post a summary note such as "LGTM :thumbsup:", or "Just a couple things to address." -- Avoid accepting a merge request before the build succeeds ("Merge when build - succeeds" is fine). -- If you set the MR to "Merge when build succeeds", you should take over +- Avoid accepting a merge request before the build succeeds. Of course, "Merge + When Build Succeeds" (MWBS) is fine. +- If you set the MR to "Merge When Build Succeeds", you should take over subsequent revisions for anything that would be spotted after that. ## Credits -- cgit v1.2.1