Pull Requests for a Better World
PRs, a response from a noob
One of the things I’m learning to do is write better pull requests (PRs). I’ve gotten through the basics of Git so now I can think about what’s actually happening on a higher level i.e. that we are collectively contributing to improve the code base in a clear and purposeful way.
Some of the things I was getting wrong was not being focused from the start. I was dumping a bunch of spaghetti code into one PR and asking for someone to review a lot of code. Mick was nice enough to point out that since I’m writing a new application, it’s common to have PRs that are much more substantial, but mine were big and disjointed. Reviewing with Daphne, I learned it was important to have pull requests that are coherent and consistent to the name of the request.
Consequently, this PR was nixed and will be broken up into more simple pieces. (https://github.com/vinomofo/MofoBottleO/pull/49)
This article confirmed the preference to smaller coherent PRs. Stotskiy says it leads to better code. (https://firstname.lastname@example.org/the-wisdom-of-splitting-task-into-subtasks-2c4b105f6b44)
One advantage that is becoming clear is the code base is of a higher quality.
I’m sure it will take time to develop my efficiency in making clear PRs but as we establish Vinomofo’s development practices further, we’ll learn what to highlight and look for in a PR workflow and review. Working with my application (MofoBottleO) I’m already anticipating we’ll have a better overall code base because we’re enforcing a lot of best practices from the beginning.
- Sandi Metz rules through Rubocop
- Continuous Integrating
- AND Mandatory Github PR approval
This article was good at highlighting what it takes to have more effective PRs (https://medium.com/@vaidehijoshi/crafting-better-code-reviews-1a5fc00a9312)
Two of the important factors were “Energy” and “Substance”
- Take time to read through and check out code
- Make it more than a formality for the team
- Take it for face value
- Save deeper feedback for face to face conversations
- Be kind
Tips I thought were cool
- Make smaller PRs
- Have PRs independent from other changes
- Use a template, ie
What does this PR do?
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
What are the relevant tickets?
Screenshots (if appropriate)
- Is there a blog post?
- Does the knowledge base need an update?
- Does this add new (Python) dependencies which need to be added to chef?
Seeing the Bigger Picture
So, I have a lot to learn but I’m glad I’m part of an Engineering Team that can see a bigger picture. Since Vinomofo has established itself as a viable company, we can get past reactionary code practices and the PR workflow is a great place to start implementing more collaborative practices.