Code review comment for lp:~openerp-dev/openobject-addons/trunk-account_check_writing_jam

Hello,

I have some remarks:

account_check_writing/__openerp__.py:
- why did you change the author, or at least removed NovaPoint ? By the way you did the same thing in another __opener__ file.

account_check_writing/account.py:
- group multiple imports from same module (from openerp.osv import osv, fields)
- tiny remark: from openerp import SUPERUSER_ID should be above from openerp.osv ... because of alphabetical order :)
- columns declaration: use line returns; 80 characters length is not a hard limit, but your lines are quite long; put the help at the next line
- 'implementation':'no_gap', -> space forgotten (use a linter)
- allow_check_writing = type == "bank" and True or False -> "and True or False" is not necessary, type == 'bank' already returns a boolean

account_check_writing/amount_to_text_en.py: I don't see the point with the whole file. Is the purpose do display '5 and 3/4' instead of '5 and 75 cents' ? Why ? If this is standard, please update the server file with comments, doc and explanations instead of re-defining everything twice for a custom addon. If this is not standard, maybe you should consider dropping this feature.

account_check_writing/account_voucher.py:
- _get_currency: no need to declare a variable user_pool that is used only once -> write a single statement
- columns :
-- check_number -> if this is the same columns as 'number', please add the 'oldname' argument to ease migration
- def get_lang(self, cr, uid, ids, context=None):
-- 1/ not sure this method is necessary and seems more like a hack than everything else; if it is necessary, please move it with other language-related methods, in tools
-- 2/ if you keep it, please avoid obfuscated code; just a language = context and context.get('lang', 'en')[0:2] seems sufficient
- def copy:
-- override default only if None, not if void
-- use update if you have multiple values to change; otherwise default[key] = value is sufficient
- def print_check: various indentation issues, please use a linter

account_check_writing/report/check_print.py
- general for this file: please add in comments the type and expected value of your various parameters; using dedicated methods for each part of the process is a good thing, but this requires some explanations of what you do and on which parameters yhou are working
- def _getchunk_line(self, defaults={}): I would put defaults=None as parameter to avoid issues with immutable dict

Best regards,

Thibault.