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

Revision history for this message
Mathieu Vatel - Julius Network Solutions (mathieu-julius) wrote :

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

=> This has been removed

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

=> done

> 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"),

=> Try to wrap most of them, (there's maybe still some of them)

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

=> I've replace all tabs inside installable modules by spaces

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

=> Done

> l.1254,1271,etc. use 'if context is None' instead of 'if context == None'

=> Done for all the installable modules

> By the way, you don't need to initialize context to {} if you do not use it
> in the body of the method

=> Of course you're right.

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

=> This should has been done

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

=> Done for the one I've found

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

=> Done

> l.1383 use inline comment with #

=> Done

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

=> No, the pack_history_id is written in the move which have been copied not in the copy.

> 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')

=> Done

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

=> Done

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

=> I've deleted them

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

=> Done

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

=> Done rev 46

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

=> Done rev 46

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

=> It's done, all the module with installable False do not work fine on the 7.0

« Back to merge proposal