Update Contributing guidelines

Fuzzbawls · Fuzzbawls · commit 4fa4cc4ff256 · 2019-06-17T14:47:48.000-07:00
Mainly punctuation and styling changes. Added a section about rebasing
pull requests.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -24,9 +24,9 @@ facilitates social contribution, easy testing and peer review.
 
 To contribute a patch, the workflow is as follows:
 
-  - Fork repository
-  - Create topic branch
-  - Commit patches
+  1. Fork repository
+  2. Create topic branch
+  3. Commit patches
 
 The project coding conventions in the [developer notes](doc/developer-notes.md)
 must be adhered to.
@@ -40,12 +40,14 @@ Commit messages should be verbose by default consisting of a short subject line
 paragraph(s), unless the title alone is self-explanatory (like "Corrected typo
 in init.cpp") in which case a single title line is sufficient. Commit messages should be
 helpful to people reading your code in the future, so explain the reasoning for
-your decisions. Further explanation [here](http://chris.beams.io/posts/git-commit/).
+your decisions. Further explanation [here](https://chris.beams.io/posts/git-commit/).
 
-If a particular commit references another issue, please add the reference, for
-example `refs #1234`, or `fixes #4321`. Using the `fixes` or `closes` keywords
+If a particular commit references another issue, please add the reference. For
+example: `refs #1234` or `fixes #4321`. Using the `fixes` or `closes` keywords
 will cause the corresponding issue to be closed when the pull request is merged.
 
+Commit messages should never contain any `@` mentions.
+
 Please refer to the [Git manual](https://git-scm.com/doc) for more information
 about Git.
 
@@ -81,7 +83,11 @@ Examples:
     Qt: Add feed bump button
     Trivial: Fix typo in init.cpp
 
-If a pull request is specifically not to be considered for merging (yet) please
+Note that translations should not be submitted as pull requests, please see
+[Translation Process](https://github.com/pivx-project/pivx/blob/master/doc/translation_process.md)
+for more information on helping with translations.
+
+If a pull request is not to be considered for merging (yet), please
 prefix the title with [WIP] or use [Tasks Lists](https://help.github.com/articles/basic-writing-and-formatting-syntax/#task-lists)
 in the body of the pull request to indicate tasks are pending.
 
@@ -94,6 +100,8 @@ At this stage one should expect comments and review from other contributors. You
 can add more commits to your pull request by committing them locally and pushing
 to your fork until you have satisfied all feedback.
 
+Note: Code review is a burdensome but important part of the development process, and as such, certain types of pull requests are rejected. In general, if the **improvements** do not warrant the **review effort** required, the PR has a high chance of being rejected. It is up to the PR author to convince the reviewers that the changes warrant the review effort, and if reviewers are "Concept NAK'ing" the PR, the author may need to present arguments and/or do research backing their suggested changes.
+
 Squashing Commits
 ---------------------------
 If your pull request is accepted for merging, you may be asked by a maintainer
@@ -102,12 +110,16 @@ before it will be merged. The basic squashing workflow is shown below.
 
     git checkout your_branch_name
     git rebase -i HEAD~n
-    # n is normally the number of commits in the pull
-    # set commits from 'pick' to 'squash', save and quit
-    # on the next screen, edit/refine commit messages
-    # save and quit
+    # n is normally the number of commits in the pull request.
+    # Set commits (except the one in the first line) from 'pick' to 'squash', save and quit.
+    # On the next screen, edit/refine commit messages.
+    # Save and quit.
     git push -f # (force push to GitHub)
 
+Please update the resulting commit message if needed, it should read as a
+coherent message. In most cases this means that you should not just list the
+interim commits.
+
 If you have problems with squashing (or other workflows with `git`), you can
 alternatively enable "Allow edits from maintainers" in the right GitHub
 sidebar and ask for help in the pull request.
@@ -120,11 +132,37 @@ the respective change set.
 The length of time required for peer review is unpredictable and will vary from
 pull request to pull request.
 
+Rebasing Pull Requests
+-------------------------
+It may become necessary for a pull request to be rebased after other pull requests have been
+merged. This is typically due to mutually exclusive changes (conflicts) between your pull
+request and the current `master` branch.
+
+When a rebase is needed, a comment will be added to the pull request indicating this need.
+Rather than simply merge the `master` branch into your pull request (which results in an
+ugly and confusing merge commit), it is better to use git's rebase feature. The basic
+workflow is as follows:
+
+    # replace 'origin' with the remote name for the main project repo in the example
+    git checkout your_branch_name
+    git fetch origin
+    git pull --rebase origin master
+
+This will "rewind" your branch commits, pull any new commits from `master`, then attempt to
+re-apply your commits on top of the new HEAD. If any conflicts are found, the process will
+pause and allow you to resolve any conflicts. Once conflicts have been resolved:
+
+    git rebase --continue
+
+Repeat as necessary until there are no more conflicts and your git tree is in a clean state.
+The final step is to push your rebased branch back up to github:
+
+    git push -f # force pushes the branch to github
 
 Pull Request Philosophy
 -----------------------
 
-Patch sets should always be focused. For example, a pull request could add a
+Patchsets should always be focused. For example, a pull request could add a
 feature, fix a bug, or refactor code; but not a mixture. Please also avoid super
 pull requests which attempt to do too much, are overly large, or overly complex
 as this makes review difficult.
@@ -148,10 +186,18 @@ There are three categories of refactoring, code only moves, code style fixes,
 code refactoring. In general refactoring pull requests should not mix these
 three kinds of activity in order to make refactoring pull requests easy to
 review and uncontroversial. In all cases, refactoring PRs must not change the
-behavior of code within the pull request (bugs must be preserved as is).
+behaviour of code within the pull request (bugs must be preserved as is).
 
 Project maintainers aim for a quick turnaround on refactoring pull requests, so
-where possible keep them short, un-complex and easy to verify.
+where possible keep them short, uncomplex and easy to verify.
+
+Pull requests that refactor the code should not be made by new contributors. It
+requires a certain level of experience to know where the code belongs and to
+understand the full ramification (including rebase effort of open pull requests).
+
+Trivial pull requests or pull requests that refactor the code with no clear
+benefits may be immediately closed by the maintainers to reduce unnecessary
+workload on reviewing.
 
 
 "Decision Making" Process
@@ -169,9 +215,9 @@ judge the general consensus of contributors.
 
 In general, all pull requests must:
 
-  - have a clear use case, fix a demonstrable bug or serve the greater good of
+  - Have a clear use case, fix a demonstrable bug or serve the greater good of
     the project (for example refactoring for modularisation);
-  - be well peer reviewed;
+  - Be well peer reviewed;
   - follow code style guidelines;
 
 Patches that change PIVX consensus rules are considerably more involved than
@@ -185,13 +231,16 @@ patches because of increased peer review and consensus building requirements.
 
 Anyone may participate in peer review which is expressed by comments in the pull
 request. Typically reviewers will review the code for obvious errors, as well as
-test out the patch set and opine on the technical merits of the patch. Project
+test out the patchset and opine on the technical merits of the patch. Project
 maintainers take into account the peer review when determining if there is
 consensus to merge a pull request (remember that discussions may have been
-spread out over GitHub, forums, email, and Slack discussions). The following
+spread out over GitHub, forums, email, and Discord discussions). The following
 language is used within pull-request comments:
 
-  - ACK means "I have tested the code and I agree it should be merged";
+  - (t)ACK means "I have tested the code and I agree it should be merged", involving
+    change-specific manual testing in addition to running the unit and functional
+    tests, and in case it is not obvious how the manual testing was done, it should
+    be described;
   - NACK means "I disagree this should be merged", and must be accompanied by
     sound technical justification (or in certain cases of copyright/patent/licensing
     issues, legal justification). NACKs without accompanying reasoning may be
@@ -209,13 +258,13 @@ that have demonstrated a deeper commitment and understanding towards the project
 (over time) or have clear domain expertise may naturally have more weight, as
 one would expect in all walks of life.
 
-Where a patch set affects consensus critical code, the bar will be set much
+Where a patchset affects consensus critical code, the bar will be set much
 higher in terms of discussion and peer review requirements, keeping in mind that
 mistakes could be very costly to the wider community. This includes refactoring
 of consensus critical code.
 
-Where a patch set proposes to change the PIVX consensus, it must have been
-discussed extensively on the forums and Slack, be accompanied by a widely
+Where a patchset proposes to change the PIVX consensus, it must have been
+discussed extensively on the forums and Discord, be accompanied by a widely
 discussed Proposal and have a generally widely perceived technical consensus of being
 a worthwhile change based on the judgement of the maintainers.
 
@@ -237,15 +286,15 @@ about:
     that personally, though! Instead, take another critical look at what you are suggesting
     and see if it: changes too much, is too broad, doesn't adhere to the
     [developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc.
-    Identify and address any of the issues you find. Then ask e.g. on Slack if someone could give
+    Identify and address any of the issues you find. Then ask e.g. on Discord if someone could give
     their opinion on the concept itself.
   - It may be because your code is too complex for all but a few people. And those people
     may not have realized your pull request even exists. A great way to find people who
     are qualified and care about the code you are touching is the
     [Git Blame feature](https://help.github.com/articles/tracing-changes-in-a-file/). Simply
     find the person touching the code you are touching before you and see if you can find
     them and give them a nudge. Don't be incessant about the nudging though.
-  - Finally, if all else fails, ask on Slack or elsewhere for someone to give your pull request
+  - Finally, if all else fails, ask on Discord or elsewhere for someone to give your pull request
     a look. If you think you've been waiting an unreasonably long amount of time (month+) for
     no particular reason (few lines changed, etc), this is totally fine. Try to return the favor
     when someone else is asking for feedback on their code, and universe balances out.
