Pratik’s Pakodas 🍿

Share this post

Better Pull Request Reviews

pakodas.substack.com

Better Pull Request Reviews

What top companies do

Pratik Bhavsar
Jan 10, 2022
4
1
Share this post

Better Pull Request Reviews

pakodas.substack.com

You don’t get anywhere if you don’t push the code. You don’t get anywhere if you push a code that fucks up. You don’t get anywhere if you push a half-fixed code.

As you keep churning code, you wonder how to maintain the technical quality of a codebase. You learn a few things here and there - SOLID, CI/CD and writing test, but is there anything more to this?

What can be a better way to think about this problem? Should I think on a higher level (abstraction) or should I think on a lower level (details)?

I am putting up a few ideas worth pursuing.

Taking the adage - “If you can't measure it, you can't improve it.” - we need to define a measure of the quality of the codebase. It requires an understanding of the evolution of codebases and the properties which make one good. I am no expert on this topic but some items which can be done are

Static code base metrics

  • test coverage ⬆️

  • avg lines per file ⬇️

  • no. of #ToDo ⬇️

Pull request metrics

  • time to merge ⬇️

  • no. of reviews done ⬇️

  • no. of comments ⬇️

  • test coverage change ⬆️

  • unit/integration/performance tests added ⬆️

Writing better code is a matter of skill x time x feedback. If you have skill but no time, you still end up with a mediocre acceptable code base at the best. It is impossible to buy time for doing PRs unless you are in a very senior role.

If you can do faster reviews, you can save time and give more feedback. This helps others to iterate quickly and yourself to spend time improving your own work. But how to do quick PR reviews?

Common

  • Everyone should be onboarded with the coding standards - often different people have a different style of writing at first. You always meet a colleague who brings camelCase out of nowhere in a Python codebase.

  • Add linting to increase the readability of the code

Author

  • Make PRs small for god sake

  • Avoid - I will fix this later mindset

  • Deploy first, refactor later. Merge when acceptable, not when perfect.

  • Don’t feel angry about the review comments

  • Do a self-review before pushing bad changes or forgotten changes

    • Did I add all documentation?

    • Did I add all type-hints?

    • Did I add all tests?

    • Did I add what was in the scope?

    • Did I add what was not in the scope?

Reviewer

  • Check the title of PR and associated task(Jira/Linear) to understand the scope

  • Review the file with the biggest change to understand the theme. Some also like to start with the tests modified

  • Go through the changes in a logical sequence

  • Fix up reminders and book time on the calendar for everyone to review

  • If the PR is big, leave some comments and request the author to break into multiple CL(changelist)

  • Non-critical or tasteful changes can be “nit:” comments and so they can be neglected by the author

  • Respond fast to comment replies

  • Last but not least - assign max time till the first review - 1 day is the industry standard. Hold people accountable if they fail at this.

  • Do commend the work if you are impressed with any implementation. PRs is not about just finding the flaws

  • Don’t use negative language

  • Enforce common tool if you see a new tool

  • Debate as per the severity


Automation

CI

  • code linter

  • import sorter

  • comment format checker

  • test runner

  • PR labeller for visibility

  • Codeowners setting (Github feature)

CD

  • deploy to dev/staging/production

  • slack/mail summaries of deployments

  • ability to deploy a particular commit for rollbacks

Self pull request review checklist

A lot of comments in PR can be avoided if the author does a self-review themselves. This led to the idea of creating an action that posts a checklist after a PR is created. Once the author checks all the things on the list, it is truly ready for review. This can save a ton of time for everyone.

Missing anything from the checklist is considered to be bad.

Github action yml

This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
on:
pull_request:
types: [opened]
jobs:
comment:
runs-on: ubuntu-latest
steps:
- uses: actions/github-script@v5
with:
script: |
github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: '**Self PR review checklist** \n- [ ] is not draft \n- [ ] add/remove dependencies \n- [ ] docstrings and type hints \n- [ ] split large func into small funcs \n- [ ] proper variable names \n- [ ] added PR label \n- [ ] optimise modules that takes more than O(n) \n- [ ] moved values to config or shared libs \n- [ ] tested on staging \n- [ ] tests \n - [ ] unit test \n - [ ] integration test \n - [ ] performance test \n - [ ] system test \n'
})
view raw pr-checklist.yml hosted with ❤ by GitHub

Come join Maxpool - A Data Science community to discuss real ML problems!

Connect with me on Medium, Twitter & LinkedIn.


Sources

https://google.github.io/eng-practices/

1
Share this post

Better Pull Request Reviews

pakodas.substack.com
Previous
Next
1 Comment
Kforcode
Jan 10, 2022Liked by Pratik Bhavsar

Awesome insights

Expand full comment
Reply
TopNewCommunity

No posts

Ready for more?

© 2023 Pratik
Privacy ∙ Terms ∙ Collection notice
Start WritingGet the app
Substack is the home for great writing