ERP5 KM

GuidelinesForBestCommitPractices

Trivia

Everything which should not be said.

  • Test your changes, by test runs and/or manually:
    • check that system is usable after change (instance is able to start, rendering is working, etc)
    • use specific tools on files (for python minimum is to run pyflakes/pylint EVEN after fixing a typo)

  • Read your patch before committing.
  • You admit being responsible of the code you commit and its effects.
  • Never commit any pdb-related code (import, call to set_trace, ...).
  • Follow the rules stated in Subversion Community Guide

The case of Business Templates

Since Business Templates are issued from automatically-generated files, the developer's control on their content is limited. Things like indentation changes happen independently from developer will. Some of the following rules can't be strictly applied to Business Templates. Still, the developer must stick to them as much as possible, for example by committing separately his changes and file format changes (indentation, ...).

Moreover, some special rules apply only to Business Templates (see below).

Commits are purpose-based

A change in the code comes from a motivation : fix a bug, add a functionality, fix typos, add comments,... This purpose must be clearly stated in the changelog message, and it must actually be the purpose itself, not how it was implemented. This way, one provides others with a way to check that a given behavior in the modified code is desired or not.

Commits are atomic

Provided such motivation exist, all the modification made on the working copy needed to fulfill this motivation must be committed at the same time. This prevent the repository from being broken during a split commit.

Commits are concise

A commit must not fulfill multiple purposes at the same time. This is done to ease the tracking of a bug by bisection methods, since it helps keeping the patches (relatively) small and helps the reviewer to keep his mind focused on one feature when understanding the changes.

Case of white spaces

A subpart of this rule needs to be explicit : about white[spaces|lines].

Basically, if the purpose is not to remove/add an empty line, then the patch must not contain such change. Same goes for python-meaningless spaces. A separate commit must be done when fixing the coding style, especially in python where some extra spaces can cause bugs. Although I believe everyone reading this page has a decent python knowledge, it's better to be safe than sorry.

A special implication of this rule is to always have a blank line at the end of files, so that unified diff does not show a diff for the last line in some cases.

Logging

The code one commits will end up in multiple contexts :

  • - Developer testing a site
    • He can accept to see many logs, but often he is only interested in his own.
    - Unit test automatons
    • Logging can be very useful to track down the problem after the test was over and the problem disappeared along with the site which generated it.
    - Production environment
    • Logging has a cost, which is slowing down zope by doing extra disk I/O. Those I/O are done synchronously and so cost much more than any other kind of disk access.
    - Newcomer trying it out,
    • Anybody stating an unknown service for the first time can get lost among the heap of log message it might happen to generate. So we must make it as easy as possible for a newcomer to understand whether the system is working at all, which is often harder to tell than developers can imagine.

Keep these "audiences" in mind when adding a log message :

  • - It must be clear enough to track it in the code (applies both to products and to ZODB) to disable it. - The log must be complete enough so that you can provide ideas of its cause by just looking at it in a unit test report. - Use wisely the log level so that any user can understand whether it's an important failure or just some execution progress symptom. Never put any log message at NORMAL (or higher) log level in a normal execution path.

Special rules for Business Templates

Always commit bt/revision

Since bt/revision is the only way to tell automatically whether the Business Template was modified and then deserves an update on a site, this file must be committed along with every change. Note that revision should always be increased.

Modify the right changelog

There are 2 changelog involved in business templates commits :

  • - One is a property of the Business Template
    • This one should contain only information about major feature change : addition of a workflow, a module, an action on a portal type, ...
    - One asked at commit time
    • This one must contain the purpose of the commit itself : refactoring, feature change, bug fix, ...

GuidelinesForBestCommitPractices (last edited 2011-07-18 12:42:47 by VincentPelletier)

Page
  • Immutable Page
  • Info
  • Attachments
User
Learn about new ERP5 releases,technical articles, events and more.

Subscribe to the monthly ERP5 Newsletter!