Code review comment for lp:~elbati/stock-logistic-warehouse/adding_stock_lot_costing

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

Well done!

Seems good to me, some nitpickings:

Most are styling and pep8 issues, but the most annoying ones are the missing propagations of the context in some calls.

Could be worse to cut the long help:

            help="Standard Price: The cost price is manually updated "
                 "at the end of a specific period. \nAverage Price: "
                 "The cost price is recomputed at each incoming shipment."),

l.195-196 (applies for other places in the code too)
pep8: indentation, see

The expression is hard to read.
I think I would prefer a version with if / elif (or maybe an inline ... if ... else ... expression but it would probably be too complex here)

l.224 pep8: missing spaces around =

l.227,241 pep8: should not be on the same line and the line is too long

l.229 propagation of the context is missing, space after commas

l.250 prefer the form `... if ... else ...` than `... and ... or ...`

l.253-256 (and some other places) the \ is useless here as it is enclosed in ()

l.257-260, l.279-290, l.305-318: propagation of context is missing (+ pep8 indentation)

l.360 and lot of calls under: propagation of the context is missing

l. 554,560,561, ... : s/incomming/incoming/

pep8: in some places, a space is missing after comma or colon.

I will be pleased to merge your proposal once fixed.

Congrats for the tests ;-)

review: Needs Fixing (code review, nitpicking, no test)

« Back to merge proposal