Vetting Patches

Owned by Former user (Deleted)
Last updated: Sept 10, 2010 by Adam MonsenVersion comment
2 min read

This document is intended to serve as a pre-submit checklist for patch contributors and a pre-commit checklist for committers. Contributions following these guidelines have a much better chance of being accepted into Mifos.

Excellent Patches

  • take less than one hour to look over and accept (not including build time)
  • were created with "git format-patch" and apply cleanly using "git am"
  • follow Code Submission Process, Coding Standards, and Vetting Patches
  • are easy to understand because they are accompanied by explanation/justification or the issue description is clear

If you know a patch is not excellent, reject it (see next section on how to do this). If you're not sure, ask for help on IRC and/or the mifos-developer or mifos-users mailing lists.

If the patch doesn't apply cleanly but is very small, manually apply it if you can do so quickly/easily.

First Pass

Initially, code should be scanned for more obvious mistakes. All unit, integration and acceptance tests pass without errors or failures. All code adheres to our standards

No "leftovers"

  • No temporary debugging code exists.
  • No commented-out code exists without explanations.
  • No generated files.

Pretty

The following ensure readability and ease of merging.

  • Code is consistently indented.
  • Imports are organized.
  • Unused imports are removed.
  • Line endings are managed by the version control system
    • This is generally not an issue with patches.
  • Present whitespace-only and formatting changes in a separate patch from changes that actually affect code execution.

Unit tests

Unit tests are...

  • suffixed with Test, like ViewSettingsTest
  • new style (Cheetah/next generation code)
    • written for TestNG
  • old style (Rhino/legacy code, and Gazelle code for now)
    • written for JUnit 4.
    • part of a test suite, like AdminTestSuite or PPITestSuite.
    • part of one of ApplicationTestSet1, 2, 3, or 4.
    • part of ApplicationTestSuite.

i18n

  • All strings are ready for translation.
  • Translations are added to _is_IS.properties for pseudo L10n.

Licensing

All files use licensing information as specified at Mifos Licensing.

Language

Proper grammar, spelling, and punctuation is used.

Ownership

Omit your own name. Ex: "this file created/mainted by me, Adam Monsen". Once the code is checked in, we all own it, any one of us may maintain it! Credit for contributors should appear in the AUTHORS file only.

This type of anonymity will:

  • increase our bus-factor
  • discourage territoriality
  • promote group ownership and responsibility

Second Pass

When delving deeper into examining a patch, consider that more complex Mifos idioms are followed and look for deeper logic issues.

Intention-Driven Code

All method names and logic is self-explanatory or has Javadoc and/or comments.

Code Reuse

Efficient, sensible, and prolific code reuse is found.

Typesafe Containers

Generics are used whenever possible to parameterize containers.

Permissions

Pages and forms are accessible/usable only by appropriate users/roles.

Dates

Date-specific and timezone-specific code is not introduced.

Configuration Files

Configuration files should be found using ConfigurationLocator. Code touched which uses another method to locate configuration files should be refactored to use ConfigurationLocator.