Three Components for Reviewing a Pull Request
A pull request is a method for submitting contributions to a software project. Maintainers or contributors review these pull requests to discuss the proposed changes and help ensure the changes meet the project guidelines and quality standards. In this talk, we will learn about three components for reviewing a pull request:
- The mechanics of code review on GitHub.
- The social aspects of code review and how to effectively give feedback.
- The technical aspects of reviewing a pull request.
The slides for this presentation are available.
Outline
- 00:00:00 Reshama introduces Data Umbrella
- 00:05:20 Thomas begins talk
- 00:06:35 Terms: pull request, reviewer, contributor, merged
- 00:07:12 PART 1: Mechanics of Code Review. Why code review?
- 00:19:10 Browsing Code in GitHub
- 00:21:50 shift+click: highlight multiple lines
- 00:22:18 GitHub b: blame, tells last contributor to the code
- 00:23:12 Overview of Mechanics
- 00:24:12 Q: How does a suggestion get incorporated into the code? how to sync local repo?
- 00:27:02 PART 2: Social Aspects of Reviewing a Pull Request
- 00:27:58 Tip 1 for giving feedback: Ask, don’t tell
- 00:28:46 Tip 2 for giving feedback: Offer alternative implementations
- 00:29:56 Tip 3 for giving feedback: Explain why code should be changed
- 00:39:56 DRY: Don’t Repeat Yourself; refactor, having common logic in multiple places, rewrite code so you have same funtionality, but this makes code more maintainable
- 00:31:50 Avoid using derogatory terms
- 00:32:32 Tip 4: Be humble, assume everyone is intelligent and well-meaning
- 00:33:15 Tip 5: Ask for clarification
- 00:34:11 Tip 6: Communicate which ideas you do not feel strongly about
- 00:35:02 Tip 7: Try to leave a positive comment
- 00:35:33 Tip 8: Thank contributors for their work
- 00:35:56 General Social Aspects
- 00:36:37 Disagreement
- 00:37:39 Talk synchronously (talk, take notes, update PR with notes to document discussion)
- 00:38:45 Overview of Social Aspects
- 00:39:05 PART 3: Technical Aspects
- 00:39:52 Presentation: how your PR is presented? description of PR
- 00:42:00 Was unrelated code changed?
- 00:42:55 Is the PR easy to review?
- 00:44:45 Engineering, Technical Aspects
- 00:44:58 Every change should be intentional
- 00:45:10 Offer ways to simplify or improve code
- 00:46:20 Does the change maintain backwards compatibility
- 00:48:42 Performance optimizations, require benchmarks
- 00:49:32 Is there documentation?
- 00:51:05 Code comments
- 00:52:15 Testing! (are there tests, and more)
- 00:54:08 What does it mean when CI tests fail?
- 00:55:42 It’s ok to make mistakes
- 00:56:53 Q&A
About the Speaker
Thomas J. Fan is a maintainer for scikit-learn, an open-source machine learning library for Python. He leads the scikit-learn project by designing the project’s roadmap, giving feedback to contributors, and implementing new features. Fan also is a maintainer for skorch, a scikit-learn compatible library that wraps PyTorch. He has a Masters in Mathematics from NYU and a Masters in Physics from Stony Brook University. During his academic studies, Fan researched quantum computation and condensed matter physics.