Patch Guidelines

1. Development environment setup

See Getting started with Noosfero development and How to Install.

2. Create an ActionItem

At the left menu of this wiki, use the 'Quick item creation' to describe a new bug or feature.

3. Branch setup

Fork official noosfero repository by clicking Clone. Then add the official noosfero remote:

git remote add noosfero https://gitlab.com/noosfero/noosfero.git
git fetch noosfero
git checkout noosfero/stable -b aiXXXX

If you are doing a bug item, then use noosfero/stable as the base. If it is a feature item, use noosfero/master as base.

For easy naming use the pattern aiXXXX, replacing XXXX with the respective ActionItem? number. You may also create your own name.

4. Code guidelines

Try hard to remember that others will read and change your code.

Principles

Keep in mind these important principles:

Communicate

English is the standard development language for ActionItems?, code, commits and everything that touches development. This helps developers from all around the world to join the Noosfero community.

For commits and action item descriptions, you don't need to use repetitive terms like must or should. Just put the sentence on the imperative form explaining the facts observed and implemented.

For commits, 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 "Fix error when doing this and that".

Focus

Focus on the change you're doing and don't mix with other unrelated stuff. The less focused your changes are the more confused the reviewer will stay.

Agregate

Agregate lines of code that does semantically similar functions.

Encapsulate

Even if the code is small, put it on an adequate function that leverages its meaning and encourages reuse.

Scope

Even if your code is on Noosfero's core, imagine it as a plugin and scope as much code as possible. This will compartmentalize and make the code more readable.

Pluginize

Consider writing your code to an existing plugin or writing a new one. If that's the way to go, read more about the plugins architecture. If the related code is on the core, consider moving it to a plugin if the effort is reasonable.

Models

Grouping or agregations is very important here, as models tend to increase in size. Look at this example model:

class Model < ActiveRecord::Base

  belongs_to :author

  has_many :childs
  has_many :parents

  named_scope :has_children
  named_scope :childs_joins, :include => [{:profile => :domains}]

  validates_presence_of :author
  validate :dont_let_this

  def self.class_method
  end

  def instance_method
  end

  protected

  def dont_let_this
  end
end

Performance

Use eager loading as much as possible, preferably directly on the associations with the :include key. Read about profiling to reduce the SQL queries and the view load time.

Views

Partialize

Partials are like methods of classes: they enable your views to be reused in other places. To make reuse even easier, use locals variables instead of instance variables.

Helpers for very small HTML and heavy logic; Partials for everything else

Don't code much HTML using helpers. They are very hard to read and maintain. Helpers should, as a rule, output very small HTML code, typically only one tag. Helpers are useful when a lot of conditions are involved when outputing a HTML. Partials and templates are applicable for everything else.

Tests

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 under the features/ directory.

Please keep in mind that the chances of getting a new feature in Noosfero without proper automated tests is very low.

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).

Translations

Write all user messages in english and them use gettext or rails i18n support to translate to your native language.

Please check Translation for more details.

Gettext

Although there is the rake updatepo tool for grabbing all user messages from code and updating all messages for all languages, in most cases you won't use it, as it will add much noise to your commits and merge request. Instead, manually add the new messages to the po/locale/noosfero.po near other messages of the same source file. Then use rake makemo to test them.

Rails i18n

Just update config/locales/locale.yml adding the translations keys using scopes and the directory structure. See this example.

Javascript

Scope your code as much as possible

Create your own classes or scope functions like the following example:
my_functionality: {

  show: function(element, url) {
  },

  hide: function(element) {
  },

  subcontext: {
    load: function(element) {
    },    
  },
};

Don't code anything directly on the views

Call a defined function with the needed parameters:
<%= link_to_function 'Click me', "my_functionality.show(this, '#{url_for :controller => :home}')" %>

<% javascript_tag do %>
  my_functionality.new_enterprise_url = '<%= url_for :controller => :escambo_plugin_myprofile, :profile => user.identifier, :action => :new_enterprise %>';
  my_functionality.subcontext.load(document.body);
<% end %>

Load locally rather globally

If your scripts aren't used in many places or by others, load them mannually on your view using javascript_include_tag. This increases performance and keep the application lighter.
<%= javascript_include_tag '/plugins/orders_cycle/javascripts/jquery.calendrical.js' %>

CSS

Scope as much as possible

Put every selector of a page under the umbrella of a parent selector. You may use the controller/action classes of the body element or create your own parent element.
#context-action .father .daughter {
}
#context-action .mother .son {
}

Avoid general selectors

General selectors creates many conflicts and the need for overwriting them in other specific places. Think many times before using element selectors or unscoped generalically named classes and identifiers, like the following:
input {
}
body * {
}
.title {
}
.available {
}
#page-title {
}

Separate functional styles from visual identity styles

If you code does things on the Noosfero's core instead of in plugins, put on public/stylesheets/application.css only properties like float, clear, position, display and all the others CSS' properties on your theme's style.css, or if it should be shared with other themes as a common visual layout, put on base theme's style.css

Use a CSS extension

Sass or LESS make your CSS code much cleaner, readable, reusable, and easier to maintain. Don't forget to commit the compiled code, as Noosfero don't use them by default.

Check Sass for binding compilation with Rails.

5. Code submission

Commit

  1. Make one commit per logical change. Please do not make merge requests full of commits that fix the previous ones.
  2. And then commit your changes: git commit (you should mention an action item, see below)
    • if you can't write your thoughts down in understandable English, ask for help. smile

Recommended reads:

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:

  Fix problem when X happens in Y
  
  (ActionItem1234)
  Implement Z 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 %SCRIPTURL{view}%/%WEB%/%TOPIC%"
  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

Request a Merge on Gitlab

If you forked Noosfero at Gitlab, you can push your commits to your Gitlab repository and make a merge request by the web site interface. Just visit Noosfero's merge requests page and click on "New merge request".

gitlab-new-merge-request.png

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
    • follow the indentation style already used (2 spaces)
    • 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.

Legacy

Generate patch — The old fashion way

Skip this section if you already went with the recommended way above (i.e. merge request on gitlab)

After you commited your change in your private repository, you may 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.

The ways to send your patch by e-mail, in order of preference:

  1. Using git send-email
  2. attach the patch file to the e-mail
  3. attach the patch file to the corresponding action item, and send the URL.
  4. attach the patch file to some random place in the internet, and send the URL.

Please try the ones in the top before using the ones in the bottom:

Further reading


Add comment
You need to login to be able to comment.
 
Topic revision: r27 - 23 Aug 2014 - 13:41:17 - BraulioBhavamitraBO

irc Talk with Devs Now!

 
Translations: English, Português
Search on Docs:
   
ActionItem Search:

Copyright © 2007-2014 by the Noosfero contributors
Colivre - Cooperativa de Tecnologias Livres