-
Notifications
You must be signed in to change notification settings - Fork 626
The Pull Request Review Process
-
🐙 I'm applying your advice about positive vs negative statements, and I updated the introduction to give it a more positive emphasis.
- ❕ Haha I love the quote "Catch bugs, because bugs happen." made me chuckle :P
-
🐙 Needs more emoji 🙂
You've already conquered one massive hurtle by writing your PR. Now it goes through the next step: the review process.
Our review process has several goals:
- Ensure high quality code, in both functionality and readability.
- Catch bugs, because bugs happen.
- Maintain consistent style so that it's easy to start working in any part of the codebase.
All of the code that goes into this repository and the core Blockly repository goes through review, whether it's written by community contributors or Blockly team members.
As reviewers, we aim to work with you to make your change as good as possible. We ask that you, as contributors, engage in conversation with us to get your pull requests through reviewed and merged.
The PR review process goes through a few stages:
When your pull request comes in, the on-call member of the Blockly team assigns a reviewer.
Reviewers are chosen based on expertise and to evenly distribute workload.
It may take a few days to get a reviewer assigned, and a few more days to get a review. Don't worry, this is normal.
During the feedback stage a reviewer leaves suggestions for changes on your PR. These could be simple things like style nits. Or they could be larger things like asking you to reorganize your function definitions.
Reviewers are encouraged to use GitHub's code reviews (rather than making individual comments) so that you receive a single notification instead of several.
The discussion phase is your chance to give feedback to the feedback. Maybe one of the review comments wasn't clear: now is your chance to ask for clarification. Or maybe your reviewer requested a change, but you think it will have repercussions: now is your chance to find a compromise.
⭐ Note: For this to work, both parties need to go into the discussion with a spirit of collaboration. The goal is not to "win" but to make something you're both proud of.
The revision phase is where you get to make changes to your PR. Usually these changes are a result of something your reviewer has said in the feedback phase. Make sure that even when you're doing revision you're following the best-practices laid out in Writing a Good Pull Request.
Once you have completed your revisions it can be helpful to tag your reviewer asking them to take another look.
Note: discussion and revision may happen at the same time.
After the revision phase your reviewer has another chance to give feedback, and the process starts from the beginning. Often a second review is simple and focuses on nits such as punctuation and code style. But sometimes a second review can be quite big. Your first reviewer may even ask someone else to take a look, to get a fresh perspective.
The review process repeats from here until you both feel happy with the state of your change.
The merge phase is your chance to celebrate.
You've done it! You've created a change, discussed and revised it, and finally gotten it merged into master! This is a grand achievement that many people never start, let alone complete!
🎉 Thank you for all of your hard work to make Blockly better. And congratulations! 🎉