Best Practices

Code Review

All patches that go into a project repo need to be code reviewed by someone other than the original author. Code review is a great way to both learn from others as well as improve code quality. Contribution to code review is highly recommended regardless of activity as a committer.

Below provides a simple checklist of common items that code reviewers should look out for (Patch submitters can use this to self-review as well to ensure that they are not hitting any of these):

General

  • Does the Git commit message sufficiently describes the change? (Refer to: https://chris.beams.io/posts/git-commit/ and https://fatbusinessman.com/2019/my-favourite-git-commit)

  • Does the commit message have an ‘Issue: <someissue>’ in the footer and not in the subject line or body?

  • Are there any typos?

  • Are all code review comments addressed?

  • Is the code rebased onto the latest HEAD of the branch?

  • Does the code pull in any dependencies that might have license conflicts with this project’s license?

Code

  • Are imports alphabetical and sectioned off by stdlib, 3rdparty, and local?

  • Are functions / methods organized alphabetically? (or categorized alphabetically)

  • Does the change need unit tests? (Yes, it probably does!)

  • Does the change need documentation?

  • Does every function added have function docs? (javadoc, pydoc, whatever the language code docs is)

  • Does it pass linting?

  • Does the code cause backwards compatibility breakage? (If so it needs documentation)

Note

Refer to Google’s blog (google-blog-code-health) on effective code review.

Generic Linting (pre-commit)

pre-commit is a Git hooks management tool and is great for running linters from any code languages. The easiest way to run pre-commit is with python-tox and requires Python 3 installed on the system:

tox -epre-commit

However if you want a more automated experience we recommend running pre-commit directly and installing the hooks such that they automatically run when you execute the git commit command. In this case, install pre-commit using your package manager or pip install it if your distro does not have it available.

Requirements

Install pre-commit

If pre-commit is not available from your native package manager than you can install it via Python’s pip install command:

pip install --user pre-commit
pre-commit --help

Once installed for every repo that are are working on you can install the pre-commit hooks directly into your local Git hooks on a per repo basis.

pre-commit install

Set up pre-commit for a Project

Requirements

Configure the project with a tox.ini and a .pre-commit-config.yaml file. Below are examples of .pre-commit-config.yaml and tox.ini as defined by this project. Inside the tox.ini file the interesting bits are under [testenv:pre-commit].

.pre-commit-config.yaml

---
default_language_version:
  python: python3

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.1.0
    hooks:
      - id: check-executables-have-shebangs
      - id: check-merge-conflict
      - id: end-of-file-fixer
      - id: trailing-whitespace

  - repo: https://github.com/jorisroovers/gitlint
    rev: v0.13.1
    hooks:
      - id: gitlint

  - repo: https://github.com/ambv/black
    rev: stable
    hooks:
      - id: black

  - repo: https://gitlab.com/pycqa/flake8
    rev: 3.8.3
    hooks:
      - id: flake8
        args: ["--max-line-length=88"]

  - repo: https://github.com/pycqa/bandit
    rev: 1.6.2
    hooks:
      - id: bandit
        # Bandit does not need to run on test code
        exclude: tests/*

  - repo: https://github.com/pycqa/pydocstyle
    rev: 5.0.2
    hooks:
      - id: pydocstyle

  - repo: local
    hooks:
      # TODO: Switch to upstream hook when https://github.com/btford/write-good/pull/119 is merged.
      - id: write-good
        name: write-good
        description: Check docs for English prose with write-good
        entry: write-good
        language: node
        files: "\\.(rst|md|markdown|mdown|mkdn)$"
        additional_dependencies: ["write-good"]
        exclude: docs/infra/gerrit.rst|docs/best-practices.rst

tox.ini

[tox]
minversion = 1.6
envlist =
    check-best-practices,
    docs,
    docs-linkcheck,
    pre-commit
skipsdist=true

[testenv]
install_command=python -m pip install --no-cache-dir {opts} {packages}

[testenv:check-best-practices]
basepython = python3
commands = python {toxinidir}/check-best-practices.py

[testenv:docs]
basepython = python3
deps = -rrequirements.txt
commands =
    sphinx-build -j auto -W -b html -n -W -d {envtmpdir}/doctrees ./docs/ {toxinidir}/docs/_build/html

[testenv:docs-linkcheck]
basepython = python3
deps = -rrequirements.txt
commands = sphinx-build -j auto -W -b linkcheck -d {envtmpdir}/doctrees ./docs/ {toxinidir}/docs/_build/linkcheck

[testenv:pre-commit]
basepython = python3
deps =
    pre-commit
commands =
    pre-commit install
    pre-commit run --all-files --show-diff-on-failure
    pre-commit run gitlint --hook-stage commit-msg --commit-msg-filename .git/COMMIT_EDITMSG

GitHub Workflow

When working directly on Github (as opposed to Gerrit systems mirrored to Github), you’ll need to create a fork and use branches/ pull requests to get changes merged to the main repo. Here are some instructions on creating and maintaining your fork.

Forking and working

  1. Fork your $PROJECT/$REPO to your personal Github account

    • NOTE if you are forking the ci-management repository you should consider

      changing the local fork name after you have forked it to be $PROJECT-ci-management this has 2 benefits:

      1. Let you know which upstream project the ci-management repo it’s for

      2. Allow you to fork the next ci-management repository that you might need to work on

  2. Clone your repo

    git clone git@github.com:$MYACCOUNT/$REPO
    
  3. Setup an upstream remote

    git remote add upstream git@github.com:$PROJECT/$REPO
    
  4. Create local branch and do your work (same as with gerrit)

    git checkout -b $feature
    
  5. Push your local branch to your fork, preferably as a branch on your fork

    git push origin $feature
    
  6. Raise PR against the upstream (note: when pushing a branch from your local to your fork the CLI gives you a URL for raising the PR)

Care and feeding of your fork

Your fork will fall out of sync with the upstream repo so be sure to tend

to your fork before working on it.

  1. Fetch upstream changes from the remote you’ve already added:

    git fetch upstream
    
  2. Switch to primary branch and refresh your master branch

    git checkout master && git pull --rebase upstream master
    
  3. Update Github with your synced fork:

    git push origin