openerp-community team mailing list archive

["Maxime Chambreuil \(http://www.savoirfairelinux.com\)" <maxime.chambreuil@xxxxxxxxxxxxxxxxxxxx>]

Maxime Chambreuil (http://www.savoirfairelinux.com) has proposed merging lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0.

Requested reviews:
  Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c): text review

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-doc/7.0-community-review/+merge/186295

[ADD] Community review guidelines
-- 
https://code.launchpad.net/~openerp-community/openobject-doc/7.0-community-review/+merge/186295
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-doc/7.0-community-review.

=== modified file 'source/contribute/05_developing_modules.rst'
--- source/contribute/05_developing_modules.rst	2013-09-17 10:47:41 +0000
+++ source/contribute/05_developing_modules.rst	2013-09-18 11:55:52 +0000
@@ -92,6 +92,7 @@
 
  * No company-related prefix or suffix in the module names (like ``c2c-account-something``);
  * Always make merge proposal for any bugfix or improvement so that everyone can take note of it and eventually ask for a different approach;
+ * Avoid resubmitting a MP if not explicitly intended. The MP will lose history of commit and that make more work for reviewers.
  * Nobody merge his/her own work into the branch. Another member of the team must do it. Exceptions may be accepted if the merge proposal has been strongly approved by the rest of the team;
  * If your module doesn’t fit into any of the available projects, or you found no related team, please post a request on the `framework expert mailing list <https://launchpad.net/~openerp-expert-framework>`_ so that we can create a specific one for you;
  * When at least one of your modules has been approved in the branch, you may ask to be part of the team. If you are not part of the team, you can still contribute to the project through merge proposals;
@@ -154,8 +155,8 @@
 If some refuse to open the projects to the community, it is always possibel to create another project.
 
 
-Misc Guidelines
-+++++++++++++++
+Community Guidelines
+++++++++++++++++++++
 
 Modules
 ^^^^^^^
@@ -206,7 +207,8 @@
 Coding Guidelines
 #################
 
-Follow Python PEP 8: http://www.python.org/dev/peps/pep-0008/
+  * Follow Python PEP 8: http://www.python.org/dev/peps/pep-0008/
+  * Do not use deprecated features
 
 Reporting
 ^^^^^^^^^
@@ -303,6 +305,22 @@
 Modules Description
 ^^^^^^^^^^^^^^^^^^^
 
+Manifest (__openerp__.py)
+
+  * Module version contains 2 numbers x.y. Increase x when the data model and other major changes. Increase y for bug fixes and other minor changes.
+  * Set the license to AGPL.
+  * Make sure the description of the module allows anyone to understand what it is used for and how to use it. Adding a scenario is good way to achieve that.
+  * The description should also explain how to setup the module (various configuration options, user settings, access rights) and specify any external library that needs to be installed.
+
+HTML Manifest (static/description/index.html)
+
+  * Should give a precise overview of what the module does (i.e. Provide a scenario, which could be illustrated by the demo data)
+  * Should not contain heavy or harmful JavaScript
+  * Should not include tracking code
+  * Should not have pointer to non ethical content
+  * Should not include commercial publicity banners
+  * Should use CSS provided by OpenERP
+
 Dependencies
 ^^^^^^^^^^^^
 
@@ -312,8 +330,8 @@
 
   * Provide only highest requirement level, not need to set ['account','base','product','sale']
 
-Module Content
-^^^^^^^^^^^^^^
+Demo data
+^^^^^^^^^
 
 Each module must contain demo data for every object defined in the module.
 
@@ -343,7 +361,7 @@
   * List of Categories -> Categories
 
 Search Criteria
-#################
+###############
 
 Search criteria: search available on all columns of the list view
 
@@ -381,3 +399,75 @@
 
     - Good: Location' Structure, Chart of Accounts, Categories' Structure
     - Bad: Tree of Category, Tree of Bill of Materials
+
+
+Community Review
+++++++++++++++++
+
+Peer review
+###########
+
+Peer review is the only way to ensure good quality of the code and to be able to rely on the others devs. The peer review in this project will be made by making Merge Proposal. It will serve the following main purposes
+
+  * Having a second look on his work to avoid unintended problems / bugs
+  * Avoid technical or business design flaws
+  * Allow the coordination and convergence of the devs by informing community of what has been done
+  * Allow the responsibles to look at every devs and keep the interested people informed of what has been done
+  * Prevent addons incompatibilities when/if possible
+
+The rationale for peer review has its equivalent in Linus's law, often phrased: "Given enough eyeballs, all bugs are shallow"
+
+Meaning "If there are enough reviewers, all problems are easy to solve". Eric S. Raymond has written influentially about peer review in software development: http://en.wikipedia.org/wiki/Software_peer_review.
+
+Basic review processes and rules
+################################
+
+Please respect a few basic rules:
+
+  * Two reviewers must approve a merge proposal in order to be able to merge it
+  * 5 calendar days or 3 working days must be given to be able to merge it
+  * A MP can be merged in less that 5 calendar days or 3 working days if and only if it is approved by 3 reviewers. If you are in a hurry just send a mail at openerp-community-reviewer@xxxxxxxxxxxxxxxxxxx or ask by IRC (FreeNode server, openobject channel).
+  * While making the merge, please respect the author using the “--author” option when committing. The author is found using the bzr log command.
+  * A bug fix must be linked to a bug report (use of --fixes=lp:#####)
+
+Functional review
+#################
+
+  * Is the module generic enough to be part of community addons?
+  * Is the module duplicating features with other community addons?
+  * Does the documentation allow to understand what it does and how to use it?
+  * Is the problem it tries to resolve adressed the good way, using good concepts?
+  * Are there some use cases?
+  * Is there any setup in code? Should not!
+  * Are there demo data?
+
+How to respond to a MP
+######################
+
+Most reference can be found here: http://insidecoding.com/2013/01/07/code-review-guidelines/
+
+
+There are the following important part in a review:
+
+  * Start by thanking the contributor / developer for his work. No matter the issue of the MP, someone make work for you here, so be thankful for that.
+  * Be cordial and polite. Nothing is obvious in a MP.
+  * The destination branch should correspond to the source of the suggesting branch.
+  * The description of the changes should be clear enough for you to understand his purpose and if apply, contain the reference feature instance in order to allow people to run and test the review
+  * Choose the review tag (comment, approve, rejected, needs information,...) and don't forget to add a type of review to let people know:
+
+    - Code review: means you look at the code
+    - Test: Means you tested it functionally speaking
+    - Code review, no test: means you just looked at the code
+
+Being picky
+###########
+
+It makes sense to be picky in the following cases:
+
+  * the origin/reason for the patch/dev is not documented very well
+  * No adapted / convenient description written in the __openerp__.py file for the module
+  * Tests or scenario are not all green and/or not adapted
+  * NO TEST at all will soon be a no go
+  * Issues with license, copyright, authorship
+  * Respect of OpenERP/community conventions
+  * Code design and best practices