This week during a code review I noticed that the code style didn't match my expectations. It's all whitespace stuff, like this:

# What was submitted for review
def function (a, b, c):
    print(a,b,c)

# What I'm accustomed to
def function(a, b, c):
    print(a, b, c)

Does whitespace matter? Yes.

Should it be discussed during a code review? No.

Aren't code reviews for discussing code?

Sure, but code reviews are about functionality and implementation.

Functionality questions are mandatory discussion points, like

• "Does the code meet the requirements?"
• "Does the code handle anticipated corner cases?"

After discussions on that level, the implementation needs to be reviewed.

• "Does the code rely on side effects?"
• "Is the intent clear when reading the code?"

This is a great opportunity to mentor developers to grow their knowledge of the language and of the codebase. For example, not everyone is aware of the itertools module. This can be spotted when the developer is managing a loop variable in a while loop. In the code below, a game can be played three times before an ad is displayed.

import itertools

# Original code
i=1
while True:
  play_game()
  if i%3==0:
    display_ad()
  i+=1

# Suggest ways to avoid manually managing that loop variable.
for i in itertools.count(1):
    play_game()
    if i % 3 == 0:
        display_ad()

# Suggest ways to clarify their intent.
for ad_required in itertools.cycle((False, False, True)):
    play_game()
    if ad_required:
        display_ad()

The developer's original code works, though it doesn't look good.

If you share language features that make that person's life easier, that's helpful.

If you say "Could you press space bar more frequently?" everyone in the room will hear "I don't like your code."

Further reading: itertools

Code style is a preference

Seriously. Code style is a preference.

There are always sound reasons to follow certain styles but those reasons typically prioritize for a particular metric.

• "Use two spaces...to avoid code running off the screen."
• "Use trailing commas...to minimize diff noise."
• "Put the opening curly brace on the same line...so folds in Notepad++ use one line instead of two."

Let's be honest, though: humans are terrible at doing the same thing repeatedly without making mistakes. With no complaints from the compiler or interpreter, nor automatic fixes from the editor, you will eventually have to choke back tears of anguish when a developer proudly shows off his fully-tested, tab-indented, objectively-terrible code.

This is where linters and style checkers really shine: they can enforce code styles so you never have to see depraved, tab-indented code again.

Where should code style get checked?

I currently recommend implementing code style checks as a pre-commit hook.

Not everybody is comfortable rebasing (or even just amending) their commits, and a pre-commit hook can help prevent new, bad code from ever getting into your VCS history.

You can implement code style checks as a unit test, but then you'll probably have to immediately start modifying tested, production code just to fix unit test errors. I strongly recommend against touching production code just to satisfy what I've already established is a preference.

Further reading: git hooks

How do I get started?

I'm familiar with pre-commit -- it's written in Python, has great documentation, and works well. To start using it, follow these steps:

1. Install pre-commit.
2. Create and commit a configuration file.
3. Install the Python pre-commit package as a git pre-commit hook.
4. Test the configuration.

In the code below, I'll use Powershell to create a sample environment and test the setup.

# 0. Create a new Python and git environment.
git init test-pre-commit
cd test-pre-commit
python -m venv venv
venv\Scripts\Activate.ps1
python -m pip install --upgrade pip setuptools
Write-Output "/venv/" > .gitignore
git add .gitignore
git commit -m "Initial commit"

# 1. Install pre-commit.
python -m pip install pre-commit

# 2. Create and commit a configuration file.
pre-commit sample-config > .pre-commit-config.yaml
git add .pre-commit-config.yaml
git commit -m "Add sample pre-commit configuration"

# 3. Install pre-commit as a hook.
pre-commit install

# 4. Test the configuration.
# This will test missing end-of-file newlines.
Write-Output "123`n456" | Out-File -NoNewLine test.txt
git add test.txt
git commit -m "Test whitespace issues"

The output I get (with current pre-commit sample configuration defaults) is:

Trim Trailing Whitespace..............................Passed
Fix End of Files......................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing test.txt

Check Yaml........................(no files to check)Skipped
Check for added large files...........................Passed

What's neat about pre-commit is that the original content will still be staged, but none of pre-commit's changes will be. This means that you can clearly see what pre-commit changed:

 123
-456
\ No newline at end of file
+456

Great, let's accept these changes and finish the commit:

git add test.txt
git commit -m "Add a text file"

Add a Python code style check

The whole point of this is to start enforcing code styles, not just whitespace styles, so let's do that now. pre-commit supports lots of checks, but for this example we're going to use black.

Using the same environment as above, add the following lines to the .pre-commit-config.yaml file:

-   repo: https://github.com/psf/black
    rev: 19.3b0
    hooks:
    -   id: black

pre-commit will complain if its configuration file has changes when other files are committed, so we'll commit it immediately.

git add .pre-commit-config.yaml
git commit -m "Configure pre-commit to use black"

Next, create a Python file with some formatting that doesn't match PEP8:

# test.py
# -------

def function (a, b, c):
    print(a,b,c)

Let's try to commit this to the repository:

git add test.py
git commit -m "Test using a Python style checker"

Now we'll see some output indicating that black has objected to the code:

Trim Trailing Whitespace.......................Passed
Fix End of Files...............................Passed
Check Yaml.................(no files to check)Skipped
Check for added large files....................Passed
black..........................................Failed
- hook id: black
- files were modified by this hook

reformatted test.py
All done!
1 file reformatted.

and test.py will be modified:

-def function (a, b, c):
-    print(a,b,c)
+def function(a, b, c):
+    print(a, b, c)

This is what PEP8-formatted code should look like, so let's commit the results:

git add test.py
git commit -m "Add a Python file"

Further reading: pre-commit hooks

Is it ever okay to discuss style in a review?

Perhaps, but for technical reasons.

If you followed the steps above, note that it's easy to add pre-commit (the Python package) as a development dependency -- just add it to your requirements file. You'll also notice that .pre-commit-config.yaml is added to the git repository, so everyone should have this file.

Unfortunately, each developer must run pre-commit install in their individual git repository. This creates the possibility that pre-commit won't run in all cases. For example, if the developer switches computers or re-clones the repository this step might be missed. This is an opportunity to discuss the setup of the development environment, though, not the style issues themselves.

It's also possible that new types of style issues will be identified. Definitely talk about this with your team! You might benefit from additional checks, or by modifying some settings in your existing checks.

Summary

Code reviews are an opportunity to mentor developers, improve code quality, and have fun as a team.

Maintain camaraderie. Delegate style enforcement to soulless automation.