Merge lp:~openerp-community/openerp-product-variant/fix-pep8-and-relative-import into lp:~anybox/openerp-product-variant/7.0

Proposed by Niels Huylebroeck on 2013-09-18
Status: Work in progress
Proposed branch: lp:~openerp-community/openerp-product-variant/fix-pep8-and-relative-import
Merge into: lp:~anybox/openerp-product-variant/7.0
Diff against target: 50 lines (+9/-10)
2 files modified
product_variant_multi/__init__.py (+1/-1)
product_variant_multi/__openerp__.py (+8/-9)
To merge this branch: bzr merge lp:~openerp-community/openerp-product-variant/fix-pep8-and-relative-import
Reviewer Review Type Date Requested Status
Guewen Baconnier @ Camptocamp (community) Needs Information on 2013-11-11
Pedro Manuel Baeza (community) code review, no test Approve on 2013-09-19
Anybox 2013-09-18 Pending
Review via email: mp+186351@code.launchpad.net

Description of the change

PEP8 + relative import

To post a comment you must log in.
Pedro Manuel Baeza (pedro.baeza) wrote :

LGTM

review: Approve (code review, no test)
Quentin THEURET @Amaris (qtheuret) wrote :

Code review ok with no tests.

But, if I run the flake8 in the branch (as described in http://pad.openerp.com/p/community-review), there are some errors : : https://cloud.theuret.net/public.php?service=files&t=325551f781f972b1eb5d088c259c07f5

Niels Huylebroeck (red15) wrote :

Quentin: cannot access that file. I will run flake8 on the module entirely and commit this, I had only done this on __openerp__.py as asked by Nicolas Bessi.

Niels Huylebroeck (red15) wrote :

A quick check with flake8 produces only one easily fixable style suggestion:
http://pastie.org/8336327

The space after the comma.

For all the other "problems" it's nagging about the 79 character limit.
Since no lines are longer than 100 characters this is a minimal discomfort imho and restructuring the code manually takes a long time.

Yes the long lines indicate a non-optimal writing. This is code that has been ported, not written from scratch.

Feel free to branch and commit (it's pushed to ~openerp-community team) and start cleaning up. Keep in mind that the larger the commit message the more time it takes to approve it.

Consider also that changing the indentation/line length will not necessarily produce cleaner code but can introduce errors in logic unless you are very careful or use automated tools.

Formatting tools (autopep8) turn the file from somewhat hard to read to impossible to read with the tons of auto-indentation it adds, I think to get a clean result you can only do it manually.

Quentin THEURET @Amaris (qtheuret) wrote :

Niels,

I expected this message :)

It was just a remark (that's why I didn't reject the MP) because the title of the branch contains fix-pep8 so I said me maybe you don't run flake8 on your code.

So, for me, the code is good.

Christophe Combelles (ccomb) wrote :

I just pushed another flake8 cleanup in our branch (most of us use a vim flake8 plugin, but we limited the line length to 100 instead of 80)

Pedro Manuel Baeza (pedro.baeza) wrote :

LGTM

review: Approve (code review, no test)

The MP's target is an Anybox's branch. Is it normal?

review: Needs Information

Unmerged revisions

200. By Niels Huylebroeck on 2013-09-18

[FIX] product_variant_multi: improve styling of __openerp__ and __init__

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'product_variant_multi/__init__.py'
2--- product_variant_multi/__init__.py 2013-04-22 22:54:34 +0000
3+++ product_variant_multi/__init__.py 2013-09-18 14:52:26 +0000
4@@ -20,4 +20,4 @@
5 #
6 ##############################################################################
7
8-import product_variant
9+from . import product_variant
10
11=== modified file 'product_variant_multi/__openerp__.py'
12--- product_variant_multi/__openerp__.py 2013-04-04 09:29:53 +0000
13+++ product_variant_multi/__openerp__.py 2013-09-18 14:52:26 +0000
14@@ -20,13 +20,13 @@
15 #
16 ##############################################################################
17 {
18- "name" : "Product Variant Multi",
19- "version" : "1.0",
20- "author" : "OpenERP SA, Akretion",
21- "category" : "Sales Management",
22+ "name": "Product Variant Multi",
23+ "version": "1.0",
24+ "author": "OpenERP SA, Akretion",
25+ "category": "Sales Management",
26 "license": "AGPL-3",
27 "summary": "Products with multi-dimension variants",
28- "description":"""
29+ "description": """
30 Multi-axial varianted product support for OpenERP
31 =================================================
32
33@@ -60,9 +60,9 @@
34 and only from product.template if not found on product.product. But at least you
35 will have been warned.
36 """,
37- "depends" : ["product"],
38- "demo" : ["demo_data.xml"],
39- "data" : [
40+ "depends": ["product"],
41+ "demo": ["demo_data.xml"],
42+ "data": [
43 "security/ir.model.access.csv",
44 "product_view.xml",
45 ],
46@@ -70,4 +70,3 @@
47 "active": False,
48 "installable": True,
49 }
50-

Subscribers

People subscribed via source and target branches