Patch Guidelines
Get noosfero source code and setup to run the tests
See
Getting started with Noosfero development
Create a new branch for your patch
git checkout -b my_hew_branch master
You can put the name you like in your branch.
Write automated tests for your code
Noosfero extensively uses automated tests to make sure we don't break anything when we need to make changes to the system. For that reason, we ask you to always write automated tests together with your patches:
- if your patch fixes a bug, make sure you have at least one automated test that fails without your fix and passes with it; you can see examples of automated tests in the
test/ directory in Noosfero source code; tests for model classes are in test/unit, tests for controllers are in test/functional, and integration tests are in test/integration. Please also consider writing automated acceptance tests (see below) if the bug or its fix has user interaction implications
- if your patch implements a new feature, make sure you write a cucumber acceptance test that explains how the user is supposed to use that feature ("the user clicks on 'Control panel', then selects option 'Content Management', then selects 'new article' ..."). You can find examples
Make sure the existing tests pass with your changes in
One of the reason why we have automated tests is to be relaxed while changing the code knowing
that if we break anything the tests will tell us that before our new code goes into a release. So, make
sure to run all existing tests (just run
`rake` in Noosfero sourcode top directory).
Commit your changes
- Add the files that you changed to the commit with:
git add path/to/changed/file.rb (repeat this step for every file you changed want to include in the patch)
- And then commit your changes:
git commit (you should mention an action item, see below)
- Write an understandable summary for your patch, describing what your patch does, not what was the problem before it. For example, instead of "Error when doing this and that", write "Fixing error when doing this and that"
- if you can't write your thoughts down in understandable English, ask for help.
Assign an action item to your patch
Every patch must be related to an existing action item, so we can track what was needed to fix a bug or implement a feature. To do
this, you should mention an action item in some part of the commit message. You can do that in the end of the message, like the following examples:
-
Fixes problem when X happens in Y
(ActionItem1234)
-
Implement ne feature
This implies the following assumptions:...
(ActionItem9999)
If you think it's hard to remember this, you can paste the following code in your
.git/hooks/commit-msg:
if ! grep -qs -e '^ActionItem[0-9]\+:' "$1"; then
echo "You must assign your commit to an existing action item!"
echo "Please refer to http://noosfero.org/Site/PatchGuidelines"
exit 1
fi
After pasting, make sure that file is executable (so git will run it before letting you commit) with:
chmod +x .git/hooks/commit-msg
Generate patch
After you commited your change in your private repository, you need to send a patch to the
development mailing list. To do that run the following command:
git format-patch master..my_branch. Git will leave a file with a name like 0001-my-commit-message.patch in your current directory, that's the patch you should send to the mailing list.
Patch review checklist
This following checklist will be followed for reviewing your patch, so it would be interesting if you do it yourself too before submitting the patch. It's not definitive, and it will certainly grow with time.
- general aspects
- the patch must not contain trailing whitespace (git show or git diff will highlight them if color is enabled)
- the patch must identify an action item in its log message
- be consistent. If you do something in some way in one place, do it the same way in all places.
- migrations
- when a new column is added and it has a default value, make sure the existing records are updated to have this default value just after the column is added (e.g.
execute("update TABLE set NEWCOLUM = DEFAULTVALUE")). That seems redundant, but not all RDBMS systems apply the default value of a new column to the existing records in the table.
- models: all application logic should be in the models. Data validations, restrictions, operations that query the database directly or modifies anything must be in the corresponding model.
- controllers: make sure controllers only do the work needed in order to implement user interaction. Let all validations and other types of logic be implemented in a model class.
- tests: avoid writing tests that take too long to run; avoid hitting the database (i.e. saving objects to it) unless absolutely necessary.
