Code review comment for lp:~mathieu-julius/stock-logistic-tracking/stock-logistic-tracking70

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Can you split your reviews into smaller chunks please?
That's a real plain to review a 6000 lines diff.

l.223-254 Why do you add 30 lines of commented code?

l.753 remove the lambda,
    'use_exist': True,

Please try to wrap the long lines to 80 chars (l.876, 877 for instance
and in _columns in general).
You can use the " continuation for that:

   'track_incoming': fields.boolean(
       'Track Incoming Lots',
       help="Forces to specify a Serial Number for all moves containing "
            "this product and coming from a Supplier Location"),

l.910-927: indentation is broken (mix tabs and spaces) (in a general
manner, check the xml files, a lot of them seems to contain tabs).

l.1170, can you remove the commented dependency if it not required anymore?

l.1254,1271,etc. use 'if context is None' instead of 'if context == None'
By the way, you don't need to initialize context to {} if you do not use it
in the body of the method

l.1269,etc. the docstring of a methods gos below the declaration, not above

l.1309,1311,etc. prefer the ternary ... if ... else form:
    move_data.name if move_data else prodlot_data.name

l.1356-1358,1482,etc. again can you clean the commented code?

l.1383 use inline comment with #

l.1419,1481 why do you not set `pack_history_id` in the `default` argument of
`copy` the line above?

l.1661-1664,1856-1861,etc. you should replace the 4 lines by using the default value of {}.get
    type = context.get('type_selection', 'product')

l.1677,1877 context.get('active_id', False)

l.1967,1968 the inline comments here are really misleading

l.2414-2418, very redundant when you can simply do active_id = context.get('active_id')

l.2780,2801,etc.: for product in product_list: (or .iterkeys())

l.2801: replace by
    product_list.set_default(x.product_id.id, 0) += x.product_qty

Do not forget to put 'installable' to True on the migrated addons.

review: Needs Fixing (code review, no test)

« Back to merge proposal