One of the main responsibilities for any BriteCore software engineer is to review Pull Requests (PR) submitted by other engineers.
Lately, we noticed we were spending a lot of time and energy on small nuisances, such as code style issues, instead of focusing our efforts on the core functionality of the PR. That's bike-shedding, and it’s harmful to the PR process.
Reviewers have only a limited amount of time and energy, and too many things to pay attention to, when reviewing a PR—security, readability, maintainability, side effects, and performance, just to name a few. Code style issues have the potential to steal the time and drain the energy of reviewers, possibly leading to inefficient reviews.
Code Style-Based Reviews are the Root of All Evil
I had to jump on the "root of all evil" bandwagon, sorry!
Pointing out code style issues is probably the easiest way to contribute to the PR author when reviewing code. But guess what? That's not super helpful.
The authors need someone to find bugs or identify better approaches for their PRs. Instead, they've got someone pointing out a missing trailing comma (when there’s a bug in the very same line that will change a report font to Wingdings).
Don't get me wrong. Consistent code style is important, but I don't believe this is something that the reviewers should spend their limited time and energy on.
Many problems may arise when a reviewer is faced with a PR full of style issues:
- The reviewer exhausts his or her energies before they even get to review the core of the PR.
- The reviewer gets that good old (and false!) feeling of a job well done before they propose any meaningful improvement.
- The reviewer gives other reviewers the feeling that the PR is in good shape just because there were several dozens of change requests already.
- Reviews may get personal, as the reviewer may try to impose their personal preferences on others. That's bad for culture and relationships.
Avoiding the above issues isn’t hard. All it takes is some effort to:
- Define a style guide.
- Promote it to the team.
- Enforce it via automation.
1. Define a Style Guide
Coming up with a good style guide for your team can be as tricky as you want it to be. My suggestion here is: instead of creating one based on your personal preferences and habits, just employ a style guide that is already widely used and documented.
Some of the advantages of reusing an existing code style guide:
- You don't spend a lot of time writing yet another style guide.
- The documentation is already written somewhere, all you need is a hyperlink (yes, I still call them hyperlinks).
- Chances are that new team members are already familiar with that style guide.
- There may be tools already available to enforce that style.
There are many style guides available for Python on the web. PEP-8 is the default style guide for Python, but many companies, such as Google, developed their own style guides that ended up becoming pretty popular as extensions to PEP-8.
If you don't want to adopt an existing code style guide as it is, you can just extend on top of an existing one.
2. Promote the Style Guide to the Team
Make sure you add a [hyper]link to the style guide to your project's README file, contribution guidelines, and your onboarding docs. This way, new people joining your team can learn about it before diving into the codebase.
And don't forget to properly announce any style guide changes to the team so they don't get caught by surprise.
3. Enforce the Style Guide Via Automation
Defining and promoting a style guide can be somewhat effective in making sure that everyone is on the same page. However, you'll still have a lot of contributions violating your style guide because:
- We're all human.
- Outside contributors may not know about them.
That's why we need automation. Once we have it set up, we'll be able to stop thinking about style, as there's a tool to do that for us.
Automate and Never Look Back
No matter what programming language or framework you're using, I am sure there are several tools built with the sole purpose of analyzing the code and pointing out style issues.
Do your team a favor—set them up. Let everyone focus on what we get paid for—building resilient software.
Python is one of the main technologies used at BriteCore. To avoid the issues I've been talking about, we have automated the code style checks, which helped to reduce bike-shedding drastically.
Our style guide is pretty simple—don't violate PEP-8, and don't worry about the rest, as our automation will catch any mistakes. :-)
We use three main tools to perform automatic verification on our Python project:
- Black: An opinionated formatter that rips code styling issues out of the discussion.
- Flake8: A linting tool/framework with several plugins to catch issues like unused variables, code complexity issues, etc.
- Isort: A linter to check and fix the order of the imports on a module.
Pre-commit ensures that the checks above will run automatically in the developer's machine before any change is committed. GitHub Actions, on the other hand, ensures that any code pushed to the remote repository follows our standards. That is helpful—especially if someone didn’t install pre-commit on their machine in the first place.
This means cleaner PRs and (almost) the end of style discussions. When an unexpected style issue comes up, we make sure to fix our setup so that next time, we have a checker in place to catch it.
Check out this sample project. It’s configured to run the tools mentioned above and can be easily applied to your Python project.
We recently experienced improvements in our PR review process by introducing style check automation. Adding these automations was pretty straightforward and paid off in only a couple of days.
The team is now more aligned on how the code should look, and we don't see PRs with dozens of comments pointing out irrelevant cosmetic details.
If your project is still lacking a style guide and the automation needed to enforce it, go ahead and spend a few hours on it. You'll find it was time well spent.