Diagrams/Dev/ReviewingAndMerging

So, you have push access to one or more of the repos in the diagrams organization on github. Now what?

• Do code reviews. When someone opens a pull request for a repository to which you have push access, review their code. (You're also welcome and encouraged to review pull requests on other repositories as well.) At a minimum this means reading carefully through their proposed changes and commenting; ideally it also means downloading and testing their code. Of course, you need not review every pull request; ideally each pull request will get 1-2 reviewers to look at it.

  Some guidelines for code reviews:

  • Be constructive. Say what is good about the patches, and how they could be improved, rather than just pointing out all the flaws. Of course, you should also be honest.
  • Be clear. Make it clear what action(s) are necessary to improve or correct the code. Clearly differentiate changes that must be made before you would consider merging the code from "nice to have" changes.
  • People are more important than code. Adjust the level and thoroughness of your comments to the committer. For long-time contributors, you can be more stringent. For new contributors, try hard to accept their patches as quickly as possible. Of course there always needs to be a basic level of correctness and style, but smaller issues can always be cleaned up later. The most important thing is for new contributors to have a positive experience, so that they want to continue contributing.
  • See it through. If you review a patch, try to stay with it through the process of revision and eventual merge. Don't demand some changes and then ignore subsequent updates. As a corollary, make sure your notification settings are such that you are aware when someone makes a new commit or comments on a pull request you have reviewed.
• Merge. When a pull request has been reviewed and the reviewers are happy with its current state (perhaps after one or more rounds of revision), merge it.

• Don't push your own changes to master. Instead, open a pull request and let others review and merge your code. Having push access doesn't mean you are above review yourself.

  You need not fork the repo, however. You are always welcome to create a new "topic branch" in the main repo and push to that branch. Then open a pull request from the branch to master (see [1]). If you prefer, you can also fork the repo and submit a pull request from your own feature branch.

  There are exceptions, however: applying this policy to every commit would get tedious. The following types of commits may be pushed to master without review:

  • Documentation fixes
  • Adjustments to dependency versions or other minor adjustments to the .cabal file (after careful testing -- don't break the build for others!)
  • Importing any file which does not affect the actual package at all; for example, documentation, notes, or some experimental code which is not (yet) actually in use.