Pull Requests for a Better World

tags: [pull-requests]


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://medium.com/@sergiy.stotskiy/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.

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”



Tips I thought were cool

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.