Difference between revisions of "Diagrams/Dev/ReviewingAndMerging"
(Update pull request policy for committers) |
m (update code review policy) |
||
| Line 1: | Line 1: | ||
So, you have push access to one or more of the repos in the [http://github.com/diagrams diagrams organization] on github. Now what? |
So, you have push access to one or more of the repos in the [http://github.com/diagrams diagrams organization] on github. Now what? |
||
| − | * <p>'''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 |
+ | * <p>'''Do code reviews'''. When someone opens a pull request for a repository to which you have push access, you are welcome to 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.</p><p>Some guidelines for code reviews:</p> |
** ''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 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. |
** ''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. |
||
Revision as of 13:15, 14 August 2013
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, you are welcome to 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.
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.
Submit your own code for review. Having push access means you are trusted to make good commits, but you are still welcome (and encouraged, for larger commits) to submit pull requests so others can review your code. Whether to submit a pull request or just push in any given situation is left up to your judgment.
Note that when submitting a pull request, you need not fork the repo; 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.