[#27] Refine multi-step PR instructions (#29)

We use multi-step format for big PRs only. Having two workflows is confusing to reviewers. Update PR workflow for all PRs to follow one format. The new format requires in every PR, * proper commit organization * use of CanIHasReview for requesting reviews
oss-generic · Feb 5, 2017 · 7d75bf6 · 7d75bf6
2 parents 28635c4 + b2f9f41
commit 7d75bf6
Show file tree

Hide file tree

Showing 5 changed files with 257 additions and 179 deletions.
diff --git a/docs/DefiningLabels.md b/docs/DefiningLabels.md
@@ -57,8 +57,8 @@ There are two types of label groups:
 
 ### Difficulty (`d.`)
 
-* `d.FirstTimers`: Easy. To do as the first issue for new developers. One developer
-  should not do more than one of these. Not to be applied for issues with a priority `p.high` or above.
+* `d.FirstTimers`: For new contributors to do as their first PR. MUST be simple enough to be contained in one commit. 
+  One developer should not do more than one of these. Not to be applied for issues with a priority `p.high` or above.
 * `d.Contributors`: Moderate difficulty. Small localized change. Can be done by contributors.
   Not to be applied for issues with a priority `p.high` or above.
 * `d.Committers`: More difficult issues that are better left for committers or more senior developers.

diff --git a/docs/FormatsAndConventions.md b/docs/FormatsAndConventions.md
@@ -41,6 +41,57 @@ Here is an example (adapted from [this commit](https://github.com/CS2103AUG2016-
  ```
 
 Refer to the article _[How to Write a Git Commit Message](http://chris.beams.io/posts/git-commit/)_ for a more detailed explanation.
+
+## Commit organization
+
+> _Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it 
+> looks good. [[source](https://twitter.com/girayozil/status/306836785739210752)]_
+
+Commits of a PR should be organized to match the following requirements:
+
+- [x] Each commit contains a single logical change, and this change must stand on its own. 
+  i.e. each commit has a single responsibility, and that responsibility must be fully carried out.<br>
+  For example, if the commit message says `Move delete() from Person class to Address class`, the commit cannot 
+  contain the addition of `delete()` to `Address` class only; it should also contain the deletion of `delete()` from
+  the `Person` class for it to be a _complete_ implementation what is stated in the commit message. <br>
+  Furthermore, the series of commits in the PR are ordered in a bottom-up fashion, each commit building 
+  on top of each other towards the end goal of the PR.
+
+  > Rationale: Reviewers should be able to review one commit at a time.
+  
+- [x] A commit should not modify more than 100 lines of code. 
+
+  > Rationale: Bigger commits make reviewing harder.
+
+  Commits containing _**mechanical changes**_ (e.g. automated refactorings, cut-paste type code movements, 
+  file renames, etc.),
+
+    * should include only one _mechanical change_  per commit.
+    * should not contain other non-mechanical changes, unless unavoidable.
+    * can exceed 100 LoC.
+    * should have the description of the change in the commit message (so that the results can be reproduced). 
+
+- [x] The build passes at each commit of the PR.
+
+  > Rationale: Build-breaking commits in the version history hinder the ability to use `git bisect` for locating bugs.
+
+- [x] Each commit has a detailed commit message which explains the context and rationale behind the commit. 
+
+  > More info: 
+  > 
+  > * [Our conventions for commit messages](#commit-message)
+  > * [Web article] _[How to Write a Git Commit Message](http://chris.beams.io/posts/git-commit/)_ 
+
+<p>
+
+> [Here](https://github.com/se-edu/addressbook-level4/pull/237) is an example of a PR that is organized 
+> as described above.
+
+<p>
+
+> **Note for first time contributors**:
+> 
+> * PRs for `d.FirstTimers` issues are usually simple enough to be contained in one commit. 
   
 ## Directory
 

diff --git a/docs/HowToGuides.md b/docs/HowToGuides.md
@@ -1,78 +1,5 @@
 # How To ...
 
-### Create a multi-step PR
-
-> Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it 
-> looks good. _[[source](https://twitter.com/girayozil/status/306836785739210752)]_
-
-Big PRs (i.e. PRs touching more than 100 lines) are hard to review. Using the _multi-step PR_ format allows the 
-review to be done incrementally.
-
-A multi-step PR consists a series of commits corresponding to a sequence of logical steps, 
-each step advancing the code base towards the end goal of the PR. The build must pass at each step. 
-
-                     
-> Build-breaking commits in the version history hinder the ability to use `git bisect` for locating bugs.
-
-[Here](https://github.com/se-edu/addressbook-level4/pull/237) is an example of a PR that is in multi-step format.
-The workflow of a multi-step PR is given below.
-
-1. Dev creates the PR branch in multi-step format.
-
-   <details>
-   <summary>A typical workflow to achieve a multi-step commit sequence (yours may differ)</summary>
-
-
-   * Plan the PR as a sequence of logical steps.
-   * Implement each step, committing each step as a separate commit. Note that you might have to deviate from the 
-     original plan along the way, which is fine; continue to commit after each significant change.
-   * After the fix is complete, refactor the commits (i.e. reorder, squash, split, etc.) to achieve  
-     a commit sequence that reflects the logical steps of the fix you should have taken in retrospect.<br>
-     Remember to write [detailed commit messages](FormatsAndConventions.md#commit-message) for each commit.
-   </details>
-
-
-1. When the PR is ready for review, dev posts a summary of commits using the 
-   [CanIHasReview tool](https://github.com/pyokagan/canihasreview/).
-
-    <details>
-    <summary>How to use CanIHasReview</summary>
-
-    1. Navigate to your PR. e.g. `https://github.com/se-edu/addressbook-level4/pull/237`.
-    1. Replace github.com in the PR URL with `canihasreview.herokuapp.com`. The resulting URL should be 
-       something like `https://canihasreview.herokuapp.com/se-edu/addressbook-level4/pull/237`.
-    1. Click `Submit new iteration` button. It will post a summary of the PR similar to 
-       [this example](https://github.com/se-edu/addressbook-level4/pull/209#issuecomment-270905049)
-
-    </details>
-       
-1. The reviewer reviews the PR in an incremental fashion.
-
-    <details>
-    <summary>Insructions for the reviewer</summary>
-
-    1. Review one commit at a time, starting with the earliest commit. 
-       [Here](https://github.com/se-edu/addressbook-level4/pull/209#pullrequestreview-15603608) is an example.
-    1. If the early commits require lot of changes, there's no need to review later commits until the early commits 
-       are updated as per review.
-
-    </details>
-      
-1. After reach round of review, the dev _updates the existing commits_  (rather than add more commits)
-   according to the review comments. 
-
-   <details>
-   <summary>A typical workflow for updating the PR in response to a review</summary>
-
-   * Commit changes in separate commits.
-   * Squash the new commits onto the relevant commits in the PR. New commits can remain if they introduce new
-     logical changes that were not in the previous version, or if the reviewer recommended splitting existing commits.
-
-   </details>
-
-1. Dev posts the summary of the new version using the same CanIHasReview tool used earlier. The reviewer is informed
-   of the new version automatically.
-
 ### Hide white space changes from being shown in the GitHub diff 
 
 Append `?w=1` to url of the `/files` page of the pull request (the "Files changed" tab)