In recent years a new code formatting tool named Black has been widely adopted by the Python community. Black formats the code for you with very little options, so the code looks the same everywhere and for everyone. Uniform code formatting makes reading the code easier, and you save time and mental energy for more important matters.
Following conversations before and during #OCAdays 2019 in Louvain-La-Neuve, a broad consensus emerged to start using Black in our Community for the 13.0 branches. So we made it happen, and took the opportunity to apply several improvements to the code linting process.
This post explains how it works and what parts of your workflow need changing (hint: not much and it's easy).
How does it work?
In the 13.0 branches of OCA repositories all code linting is now done using pre-commit.
pre-commit is a tool that let us configure Black and all linters in each repository, and makes it very easy for contributors to run them locally, in exactly the same way as it runs in CI (Travis).
This new approach has several important benefits:
Local installation of the linters is much easier: only one tool needs to be installed on your machine: pre-commit, which will then take care of installing all the required linters for you.
After running "pre-commit install" once in a git clone directory, lint checks are then run automatically when you commit locally so you can detect errors much earlier, without having to wait for Travis to detect them for you: you save time, and precious Travis resources.
Some linters (such as Black, and the import sorter) fix the code for you, so you immediately have the diff available with all fixes to commit.
All configurations are stored locally in the repository using default configuration files (.pylintrc, .flake8, etc); this means your favorite IDE will pick them up automatically to give you assistance in detecting and fixing errors.
A wide variety of pre-commit plugins (aka hooks) exist, which means we can easily benefit from them and progressively add more in the future.
So what do I need to do in practice?
This new mechanism is very easy to use.
First, you need to install pre-commit following the instructions. This needs to be done once. For your comfort, make sure the pre-commit command is in your PATH.
Then, each time you clone an OCA repository to work on a 13.0 branch, run "pre-commit install" to create a git commit hook (add -f for replacing any previous pre-commit config). It will run automatically each time you git commit, to make sure all rules are enforced, and even apply some automatically for you.
Finally, it you may want to configure Black in your favorite code editor, so standardized code formatting is just one keyboard shortcut away. If you don't, this is fine too: when you commit, pre-commit will run Black and isort for you (among other things), modify the code and warn if there are some changes to be applied. You can then simply "git add -u" and "git commit" again.
How to migrate addons to 13.0?
The migration procedure for 13.0 has been adapted to take the new mechanism into account.
An important part is to create a separate commit that does the initial Black/isort/pyupgrade automatic formatting on an existing code base. After the initial formatting has been done once, it become parts of the normal workflow and is done automatically with regular development commits.
If you already have a migration pull request that was created before the new configuration was in place:
Add the OCA repository as a remote (normally it should be origin).
Fetch that remote: git fetch origin.
Place on your migration branch: git checkout <branch>.
Rebase your branch over latest content: git rebase origin/13.0.
Run pre-commit for all (assuming you have already installed it): pre-commit run -a.
Depending on the circumstances, you may want to create a separate commit with the autoformatting, or amend your migration commit: git commit --all --amend.
What are the new checks and rules?
Since we now have a much easier mechanism to run checks and make them much more visible to contributors, we took the opportunity to enforce and automate some rules that where showing up frequently in code review. Here is the complete list, as of this writing (2019-10-13):
(new) Code formatting with Black, using all default options (including quote normalization to ", and the default maximum line length of 88 characters).
(new) Sorting of import statements with isort and seed-isort-config. Imports are automatically sorted in 5 main blocks: standard library, third party packages, odoo, odoo.addons, local imports.
Run pylint with all usual pylint-odoo checks (non blocking in case of error).
Run pylint with pylint-odoo mandatory checks. The list of mandatory rules has been extended to include the most common errors that normally don't generate false positives.
flake8 (as before), and flake8-bugbear (new).
(new) pyupgrade for using newer python syntax
(new) Trim trailing whitespace at end of line
(new) Fix newline at end of file
(new) Use linux line endings (LF)
(new) Remove python file encoding pragma (# -*- coding: utf-8 -*-)
(new) check for broken symlinks
(new) check xml syntax
(new) checks for a common error of placing code before the docstring
(new) check that executables start with #!
(new) check for debug statements in the code (e.g. import pdb; pdb.set_trace())
(new) check for unresolved merge conflicts in the code
(new) check for files with names that would conflict on a case-insensitive filesystem
eslint, with all checks as warnings, as was previously done by pylint-odoo
What if some checks cause problem in my code?
As before, we aim at using the same checks for all OCA repositories.
All the new checks should be fairly uncontroversial, and either applied automatically or easy to fix manually.
If some linter raises what you think are false positive, or otherwise misbehaves, there are several things you can do:
First make sure you understand the check, and ask for help in your pull request. If you want to bypass some check and commit anyway, you can use "git commit --no-verify". The error will be picked up by Travis and you can discuss it in the PR.
If the check needs to be bypassed for a good reason, use the corresponding linter method to disable it locally. Most of them support local comments to disable some checks or formatting (Black # fmt: on/off, pylint # pylint: disable, flake8 # noqa, isort # isort: skip), look at their respective documentation for details.
As a last resort, the linter configuration files may need to be adapted. In such cases, be sure to make a Pull Request to OCA/maintainer-quality-tools to propose and discuss the change. In case of emergency, local changes to the configuration files can be made in the repository. Be aware however that in the future, the OCA GitHub bot may enforce linter configuration to be identical for all repos.
