Code Submission conventions

In order to have a clean and maintainable repository, every developer contributing the code to OpenLMIS is asked to follow some simple rules. Those rules describe how to name the commits or what a single commit should contain. Having clean repository helps during code reviews as well as when trying to find a specific change.

Commit title/message body conventions

OLMIS-1234 Short description of the work
<empty line>
Longer description of the work, unless it's completely trivial.
Should explain what has been implemented and why,
rather than how.

The story number should be placed in the commit title (if the commit is related to a ticket).
The description should contain a very brief description of the actual work that is included in this commit. So if the commit just adds the domain for Order, simply write "Add order domain". Avoid just copy-pasting the ticket title, which usually does not map 1:1 to what has been implemented.

Most of the commits should also contain a body message. The commit body message should focus on explaining what has been implemented and why, rather than how (since one can just browse the code if they are interested in how something was done). When looking through commits the most common question is "why it has been done?" and having a commit body explaining that can often help save some time or avoid regression. Having the commit body message can also be helpful during code reviews, since it may already answer some of the reviewer questions (why do we need this?).

As a reference, please take a look at: http://chris.beams.io/posts/git-commit/#seven-rules

Commits with code review incorporation

The commits that incorporate the code review comments should follow the same pattern as described above. We should avoid having commits saying "Code review fixes", unless those changes are completely trivial (but even then it's usually easy to come up with better commit title). The commit body message can be skipped if no explanation for changes is necessary or the title already explains everything, eg. "Fix typo in RequisitionService" or "Adjust method names in OrderRepository".

Work on master branch using git pull --rebase

We recommend using the master branch for development rather than feature branches. This is our current practice during active OpenLMIS development. Why? Pushing to master is the only way to get CI/CD to run tests and deploy to a server where others can test drive our work easily, so doing that often helps us all keep in sync during active development. Feature Branches fit with a pull request workflow, which we aren’t using now.

Our recommended git workflow is:

git checkout master  #work on master branch
...
git commit                     #commit locally any time
...
git pull --rebase       #use --rebase every time you pull
git push                         #what you push will be clearer, without merges

For more details about this workflow and using git pull --rebase, see the Git Rebase Slides.

Squashing commits

While it's OK to commit small bits of work often on your local repo (or on a development branch), it should be avoided pushing the same directly to the GitHub master branch. The main reason for this is that the commits on your local repo or development branch often contain stuff that is later on removed or modified in the next commits. This creates mess in the tree and makes browsing through the changes/determining what was actually modified really difficult. Therefore, once the feature/fix is complete, when you are ready to push to GitHub master (or merge the development branch into master), the commits should be squashed and the commit title/body naming conventions should be then applied to that squashed commit.

For info about how to squash commits using git rebase -i (interactive rebase), see Interactive Rebasing—Atlassian Git Tutorial.

Single commit should always represent a single, completed chunk of work. We should avoid mixing in unrelated stuff - eg. combining two tickets together or adding in a major refactor. Having some changes in one class that has also undergone a major refactor makes it hard to review and determine which parts were added/modified and which were only refactored. Squashing commits together well can help the reviews stay focused and clear. As you choose which commits to squash together, the idea is to squash for atomicity, cleanliness and to make the code history readable during review.

Development branch naming strategy

Currently using development branches is discouraged (see above). But if you decide to do it, please give the branch a name that includes the ticket number and a short meaningful name. Adding just 2-3 words to the branch name can help to checkout branches easily. Some of the existing branches that follow this pattern are: origin/OLMIS-240-stock-management-logic  or  origin/OLMIS-860-restAutomaticTest.

Commits with breaking changes

Code that breaks the build is not acceptable. In case developers expect they may not be able to fix Sonar/tests/build issues within a working day, breaking changes should be reverted.