Merge lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr into lp:~margin-analysis-core-editors/margin-analysis/7.0

[Yannick Vaucher @ Camptocamp]

Pre-portage only retabing XML for readability

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

LGTM, Thanks !

review: Approve (code review, no tests)

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Yannick, thank you very much for the cleaning.

I wonder, because we have never discussed this before, what is the correct space indentation for XML. In Python it is used 4 spaces. I have seen that you have used 2 spaces. I see this a little short, but I would like to know the opinion of others.

Regards.

review: Needs Information (code review, no test)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

We do not really define anything regarding the XML file... I will be in favor of having the same space identation : 4 spaces.

Regards,

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

I prefer 2 spaces indentations as in xml you can have a lot of indentation levels
In python with max width of 80 or 100 and for readability you better have a maximum of 5 indentation level and can have only 3 levels.

class:
4s def:
8s code
12s nested code

In XML if you want to do inheritance you will need at least 6 levels

<openerp>
4s <data>
8s <record id=
12s <field name="arch"
16s <xpath
20s <field name="my_new_field"

Furthermore, with html and v7 style you get even more indentation levels

Let's take the kanban view of res.partner as a bad example:
line 237 in res_partner_view.xml
it begins at columns 77
(19 levels of indentation and if should be 20 levels as sheet tag isn't properly indented)

In my opinion, 2 spaces in XML doesn't make it less readable.

But I don't intend to indent by 2 if we choose 4 for our code.

3 spaces would be a compromise and tabs would let people decide how to display it and reduce file size.
But let's keep the discution on 2 or 4 as it would start being more philosophical :)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

I vote then for 2 spaces

[Pedro Manuel Baeza (pedro.baeza)]

Then maybe we have to tell to whole community and get an agreement to use this convention across all the contributions. Don't you think?

Regards.

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

+1 to add this in the community docs Maxime wrote a little while ago.

On Thu, Oct 17, 2013 at 1:35 PM, Pedro Manuel Baeza
<email address hidden>wrote:

> Then maybe we have to tell to whole community and get an agreement to use
> this convention across all the contributions. Don't you think?
>
> Regards.
> --
>
> https://code.launchpad.net/~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr/+merge/190350
> You are reviewing the proposed merge of
> lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr
> into lp:margin-analysis.
>

--

*camptocamp*
INNOVATIVE SOLUTIONS
BY OPEN SOURCE EXPERTS

*Joël Grand-Guillaume*
Division Manager
Business Solutions

[phone]
www.camptocamp.com

[Stefan Rijnhart (Opener) (stefan-opener)]

With regards to XML tab width: the defacto standard in official addons is 4 throughout even if there are some inconsistencies. I vote for sticking to 4 for that reason, even if 2 would have been more practical.

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

About OpenERP "standard" of former code we also have import with bad namespace, use of class alias and useless class instanciations.

In OpenERP coding guidelines nothing is clearly said about XML indentation:
https://doc.openerp.com/7.0/contribute/15_guidelines/
As you said, this is defacto

Nevertheless, my opinion is that we shouldn't say "this is a standard" if there are no obvious reasons beside it.

So what will be the community standard we will decide and why?

By the way, readability counts
(Does it also counts on code review in lp?)

[David BEAL (ak) (davidbeal)]

I vote then for 2 spaces

more readeable for new use case (kanban, futures cases)

[Bogdan Stanciu (bstanciu)]

2 spaces for reasons given above. thanks

[Ronald Portier (ronald-portier)]

I also vote for 2 spaces.

Also please: if there is more then one attribute, have each attribute on a new line.

(Unfortunately OpenERP will break if you spread attributes over multiple lines, even where this would be perfectly logical, as with long domain expressions).

Having each attribute on a new line will make changes much more "diff friendly". if you change one attribute, instead of having to scan a very long line where exactly anything changed, this will be immediately obvious.

[lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr]

48. By Joël Grand-Guillaume @ camptocamp on 2013-11-14

[FIX] Function signature addiing context

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

We go for 2 spaces then it seems. Can we go further on this merge please ?

=== modified file 'product_historical_margin/account_invoice_view.xml'
--- product_historical_margin/account_invoice_view.xml	2012-07-12 13:56:17 +0000
+++ product_historical_margin/account_invoice_view.xml	2013-11-14 15:32:34 +0000
@@ -1,81 +1,80 @@
 <?xml version="1.0" encoding="utf-8"?>
 <openerp>
   <data>
-      
-       <record id="view_invoice_line_tree_from_product" model="ir.ui.view">
-              <field name="name">account.invoice.line.from.product.tree</field>
-              <field name="model">account.invoice.line</field>
-              <field name="type">tree</field>
-              <field name="priority">20</field>
-              <field name="arch" type="xml">
-                  <tree string="Invoice Line">
-                      <field name="invoice_type"/>
-                      <field name="partner_id"/>
-                      <field name="invoice_user_id"/>
-                      <field name="invoice_id"/>
-                      <field name="invoice_date"/>
-                      <field name="name" invisible="1"/>
-                      <field name="product_id"/>
-                      <field name="account_id" invisible="1"/>
-                      <field name="quantity"/>
-                      <field name="uos_id"/>
-                      <field name="price_unit" invisible="1"/>
-                      <field name="discount" invisible="1"/>
-                      <field name="price_subtotal" invisible="1"/>
-                      <field name="subtotal_cost_price" invisible="1"/>
-                      <field name="company_id" invisible="1"/>
-                      <field name="subtotal_company"/>
-                      <field name="subtotal_cost_price_company"/>
-                      <field name="margin_absolute"/>
-                      <field name="margin_relative" invisible="context.get('group_by',False)"/>
-                  </tree>
-              </field>
-          </record>
-          
-        
-      <record id="view_account_invoice_line_filter" model="ir.ui.view">
-          <field name="name">account.invoice.line.select</field>
-          <field name="model">account.invoice.line</field>
-          <field name="type">search</field>
-          <field name="arch" type="xml">
-              <search string="Search Invoice Line">
-                 <group>
-                     <filter icon="terp-go-year" string="Year" 
-                             name="year"
-                             domain="[('invoice_date','&lt;=', time.strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',time.strftime('%%Y-01-01'))]"
-                             help="year"/>
-                     <separator orientation="vertical"/>
-                     <filter icon="terp-go-month" string="Month"
-                             name="month"
-                             domain="[('invoice_date','&lt;=',(datetime.date.today()+relativedelta(day=31)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today()-relativedelta(day=1)).strftime('%%Y-%%m-%%d'))]"
-                             help="current month"/>
-                     <filter icon="terp-go-month" string="Month-1"
-                             domain="[('invoice_date','&lt;=', (datetime.date.today() - relativedelta(day=31, months=1)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today() - relativedelta(day=1,months=1)).strftime('%%Y-%%m-%%d'))]"
-                             help="last month"/>
-                     <separator orientation="vertical"/>
-                      <field name="product_id"/>
-                      <field name="invoice_type"/>
-                      <field name="partner_id"/>
-                      <field name="invoice_user_id"/>
-                      <field name="invoice_date"/>
-                      <field name="account_id"/>
-                      <field name="company_id"/>
-                  </group>
-                  <newline/>
-                  <group expand="1" string="Group By...">
-                      <filter string="Partner" icon="terp-partner" domain="[]" context="{'group_by':'partner_id'}"/>
-                      <filter string="Salesman" name='user' icon="terp-personal" context="{'group_by':'invoice_user_id'}"/>
-                      <separator orientation="vertical"/>
-                      <filter string="Account" icon="terp-folder-orange" domain="[]" context="{'group_by':'account_id'}"/>
-                      <filter string="Invoice" icon="terp-folder-orange" domain="[]" context="{'group_by':'invoice_id'}"/>
-                      <separator orientation="vertical"/>
-                      <filter string="Invoice Type" icon="terp-stock_effects-object-colorize" domain="[]"  context="{'group_by':'invoice_type'}"/>
-                      <filter name="product" string="Product" icon="terp-accessories-archiver" context="{'group_by':'product_id'}"/>
-                  </group>
-             </search>
-          </field>
-      </record>
+
+    <record id="view_invoice_line_tree_from_product" model="ir.ui.view">
+      <field name="name">account.invoice.line.from.product.tree</field>
+      <field name="model">account.invoice.line</field>
+      <field name="type">tree</field>
+      <field name="priority">20</field>
+      <field name="arch" type="xml">
+        <tree string="Invoice Line">
+          <field name="invoice_type"/>
+          <field name="partner_id"/>
+          <field name="invoice_user_id"/>
+          <field name="invoice_id"/>
+          <field name="invoice_date"/>
+          <field name="name" invisible="1"/>
+          <field name="product_id"/>
+          <field name="account_id" invisible="1"/>
+          <field name="quantity"/>
+          <field name="uos_id"/>
+          <field name="price_unit" invisible="1"/>
+          <field name="discount" invisible="1"/>
+          <field name="price_subtotal" invisible="1"/>
+          <field name="subtotal_cost_price" invisible="1"/>
+          <field name="company_id" invisible="1"/>
+          <field name="subtotal_company"/>
+          <field name="subtotal_cost_price_company"/>
+          <field name="margin_absolute"/>
+          <field name="margin_relative" invisible="context.get('group_by',False)"/>
+        </tree>
+      </field>
+    </record>
+
+
+    <record id="view_account_invoice_line_filter" model="ir.ui.view">
+      <field name="name">account.invoice.line.select</field>
+      <field name="model">account.invoice.line</field>
+      <field name="type">search</field>
+      <field name="arch" type="xml">
+        <search string="Search Invoice Line">
+          <group>
+            <filter icon="terp-go-year" string="Year" 
+                name="year"
+                domain="[('invoice_date','&lt;=', time.strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',time.strftime('%%Y-01-01'))]"
+                help="year"/>
+            <separator orientation="vertical"/>
+            <filter icon="terp-go-month" string="Month"
+                name="month"
+                domain="[('invoice_date','&lt;=',(datetime.date.today()+relativedelta(day=31)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today()-relativedelta(day=1)).strftime('%%Y-%%m-%%d'))]"
+                help="current month"/>
+            <filter icon="terp-go-month" string="Month-1"
+                domain="[('invoice_date','&lt;=', (datetime.date.today() - relativedelta(day=31, months=1)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today() - relativedelta(day=1,months=1)).strftime('%%Y-%%m-%%d'))]"
+                help="last month"/>
+            <separator orientation="vertical"/>
+            <field name="product_id"/>
+            <field name="invoice_type"/>
+            <field name="partner_id"/>
+            <field name="invoice_user_id"/>
+            <field name="invoice_date"/>
+            <field name="account_id"/>
+            <field name="company_id"/>
+          </group>
+          <newline/>
+          <group expand="1" string="Group By...">
+            <filter string="Partner" icon="terp-partner" domain="[]" context="{'group_by':'partner_id'}"/>
+            <filter string="Salesman" name='user' icon="terp-personal" context="{'group_by':'invoice_user_id'}"/>
+            <separator orientation="vertical"/>
+            <filter string="Account" icon="terp-folder-orange" domain="[]" context="{'group_by':'account_id'}"/>
+            <filter string="Invoice" icon="terp-folder-orange" domain="[]" context="{'group_by':'invoice_id'}"/>
+            <separator orientation="vertical"/>
+            <filter string="Invoice Type" icon="terp-stock_effects-object-colorize" domain="[]"  context="{'group_by':'invoice_type'}"/>
+            <filter name="product" string="Product" icon="terp-accessories-archiver" context="{'group_by':'product_id'}"/>
+          </group>
+        </search>
+      </field>
+    </record>
 
   </data>
 </openerp>
-

=== modified file 'product_historical_margin/invoice.py'
--- product_historical_margin/invoice.py	2012-11-14 15:53:00 +0000
+++ product_historical_margin/invoice.py	2013-11-14 15:32:34 +0000
@@ -26,10 +26,11 @@
 class account_invoice(Model):
     _inherit = 'account.invoice'
 
-    def _refund_cleanup_lines(self, cr, uid, lines):
+    def _refund_cleanup_lines(self, cr, uid, lines, context=None):
         for line in lines:
-            del line['invoice_user_id']
-        return super(account_invoice, self)._refund_cleanup_lines(cr, uid, lines)
+            line.invoice_user_id = False
+        return super(account_invoice, self)._refund_cleanup_lines(cr, uid, 
+          lines, context=context)
 
 
 class account_invoice_line(Model):

=== modified file 'product_historical_margin/product_view.xml'
--- product_historical_margin/product_view.xml	2012-11-14 15:53:00 +0000
+++ product_historical_margin/product_view.xml	2013-11-14 15:32:34 +0000
@@ -9,8 +9,8 @@
       <field name="type">form</field>
       <field name="arch" type="xml">
         <field name="standard_margin_rate" position="after">
-            <field name="margin_absolute" />
-            <field name="margin_relative" />
+          <field name="margin_absolute" />
+          <field name="margin_relative" />
         </field>
       </field>
     </record>

=== modified file 'product_historical_margin/wizard/historical_margin_view.xml'
--- product_historical_margin/wizard/historical_margin_view.xml	2012-07-12 13:56:17 +0000
+++ product_historical_margin/wizard/historical_margin_view.xml	2013-11-14 15:32:34 +0000
@@ -1,43 +1,42 @@
 <?xml version="1.0" encoding="utf-8"?>
-
 <openerp>
-    <data>
-        <record id="view_historical_margin" model="ir.ui.view">
-            <field name="name">historical.margin.form</field>
-            <field name="model">historical.margin</field>
-            <field name="type">form</field>
-            <field name="arch" type="xml">
-                <form string="Historical Margin Properties">
-                    <group colspan="4">
-                        <field name="from_date" />
-                        <field name="to_date"/>
-                        <field name="product_ids" invisible="1"/>
-                    </group>
-                    <separator string="" colspan="4"/>
-                    <group colspan="4" col="6">
-                        <button icon="gtk-cancel" special="cancel" string="Cancel"/>
-                        <button icon="terp-gtk-go-back-rtl" string="Compute margins" name="action_open_window" type="object"/>
-                    </group>
-                </form>
-            </field>
-        </record>
-
-        <record id="action_product_margin_view" model="ir.actions.act_window">
-            <field name="name">Product Historical Margin</field>
-            <field name="res_model">historical.margin</field>
-            <field name="view_type">form</field>
-            <field name="view_mode">tree,form</field>
-            <field name="view_id" ref="view_historical_margin"/>
-            <field name="target">new</field>
-        </record>
-        
-        
-        <menuitem icon="STOCK_INDENT" action="action_product_margin_view"
-            id="menu_action_product_margin_tree"
-            parent="account.menu_finance_statistic_report_statement" />
-
-        
-        <!-- Remove it for right management <act_window name="Product Historical Margins"
+  <data>
+    <record id="view_historical_margin" model="ir.ui.view">
+      <field name="name">historical.margin.form</field>
+      <field name="model">historical.margin</field>
+      <field name="type">form</field>
+      <field name="arch" type="xml">
+        <form string="Historical Margin Properties">
+          <group colspan="4">
+            <field name="from_date" />
+            <field name="to_date"/>
+            <field name="product_ids" invisible="1"/>
+          </group>
+          <separator string="" colspan="4"/>
+          <group colspan="4" col="6">
+            <button icon="gtk-cancel" special="cancel" string="Cancel"/>
+            <button icon="terp-gtk-go-back-rtl" string="Compute margins" name="action_open_window" type="object"/>
+          </group>
+        </form>
+      </field>
+    </record>
+
+    <record id="action_product_margin_view" model="ir.actions.act_window">
+      <field name="name">Product Historical Margin</field>
+      <field name="res_model">historical.margin</field>
+      <field name="view_type">form</field>
+      <field name="view_mode">tree,form</field>
+      <field name="view_id" ref="view_historical_margin"/>
+      <field name="target">new</field>
+    </record>
+
+
+    <menuitem icon="STOCK_INDENT" action="action_product_margin_view"
+        id="menu_action_product_margin_tree"
+        parent="account.menu_finance_statistic_report_statement" />
+
+
+    <!-- Remove it for right management <act_window name="Product Historical Margins"
                     res_model="historical.margin"
                     src_model="product.product"
                     view_mode="form"
@@ -46,5 +45,5 @@
                     id="historical_margin_act_window"/> -->
 
 
-    </data>
+  </data>
 </openerp>