Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Joël Grand-Guillaume @ camptocamp
Review: Approve text review

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

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Maxime Chambreuil (http://www.savoirfairelinux.com)
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 [hidden email] 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


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Guewen Baconnier @ Camptocamp
In reply to this post by Joël Grand-Guillaume @ camptocamp
Review: Needs Fixing

Some comments if you have ideas how to improve them:

--

I find this sentence a bit obscure:

"Avoid resubmitting a MP if not explicitly intended. The MP will lose history of commit and that make more work for reviewers."

I would prefer but still I'm not sure:
"Avoid to use the 'resubmit' link on the merge proposals if not asked."

--

Could we emphasize that the HTML manifest is not mandatory?

--

5 calendar days or 3 working days must be given to be able to merge it

Can probably be replaced by "3 working days must be...", the 5 calendar days precision is useless.

--

"The destination branch should correspond to the source of the suggesting branch."
While I think I understand this sentence, I find it awkward. I suggest to remove this sentence because anyway bzr won't allow to merge a branch in a branch with a different origin.

--

"NO TEST at all will soon be a no go"
Ambiguous.
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Stefan Rijnhart (Therp)
In reply to this post by Joël Grand-Guillaume @ camptocamp
Review: Needs Information

Thank you Maxime, for putting this up after the collective input was gathered on the pad!

Could you split off l.108 in a separate stanza about things to consider when committing an approved proposal to its target branch?

Also I would like to see l.109 make explicit that it concerns the proposed branch, so that it can not be understood as encouraging the merging party to link the target branch to the bug report at merge time.

l.148 Maybe "NO TEST at all will soon be a no go" should say "Having tests is very much encouraged" until the OCA decides otherwise.

l.138 does not really add anything to l.136. Saying 'Code review, no test' is just a more emphatic way of saying 'Code review', right?

@Guewen: I still think the 3 working days is not very meaningful in a world wide community, so I would prefer to go the other way and drop this terminology in favour of 5 calendar days.

Nothing in this proposal is factually wrong and probably subject to taste, so I'll set my review to 'Needs Information' rather than 'Needs fixing'
--
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Guewen Baconnier @ Camptocamp
On 09/18/2013 04:04 PM, Stefan Rijnhart (Therp) wrote:
> @Guewen: I still think the 3 working days is not very meaningful in a world wide community, so I would prefer to go the other way and drop this terminology in favour of 5 calendar days.

Good point. Anyway we should drop one or the other, I agree to keep the
5 calendar days.

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

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Stefan Rijnhart (Therp)
In reply to this post by Joël Grand-Guillaume @ camptocamp
Thanks for picking up the changes! I am afraid I forgot to mention another one: in line 40, you mention that the license should be set to 'AGPL'. Maybe change this to 'AGPL-3' to reflect that this is the exact entry in the module model's selection field. Modules with a license literally set to 'AGPL' give an error when you try to install them.



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

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Guewen Baconnier @ Camptocamp
In reply to this post by Joël Grand-Guillaume @ camptocamp
Thanks for the changes.

In the "Merging" section, I think it could be useful to remind that the commit message defined (if defined) on the MP should be used. It can seems obvious, but I'm not sure that everyone is aware of that. Conversely, ideally the person who propose a merge should also propose a commit message (rarely used actually).
--
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Stefan Rijnhart (Therp)
In reply to this post by Joël Grand-Guillaume @ camptocamp
Review: Approve

Thanks for the changes!
--
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Maxime Chambreuil (http://www.savoirfairelinux.com)
In reply to this post by Joël Grand-Guillaume @ camptocamp
The proposal to merge lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0 has been updated.

Commit Message changed to:

[MRG] Add community review guidelines

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-doc/7.0-community-review/+merge/186295
--
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

Guewen Baconnier @ Camptocamp
In reply to this post by Joël Grand-Guillaume @ camptocamp
Review: Approve

Thanks for having took care of that.
Seems good to me.

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

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0

noreply
In reply to this post by Joël Grand-Guillaume @ camptocamp
The proposal to merge lp:~openerp-community/openobject-doc/7.0-community-review into lp:~openerp-community/openobject-doc/v7.0 has been updated.

    Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-doc/7.0-community-review/+merge/186295
--
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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] this is worth seeing

debaetsr
In reply to this post by Joël Grand-Guillaume @ camptocamp
Hello!

I've just found something  really  strange but at  the same time interesting,  I think it is worth seeing, just take a look <http://credible.kitefamily.us/e4vnfm>


Warmest regards, ruben



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

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp