openerp-community-reviewer team mailing list archive

[Alexis de Lattre <alexis@xxxxxxxxxx>]

@guewen
I took into account almost all your remarks :
- I add the _prepare functions (it was in my roadmap to develop them :)
- I now use assert instead of a dirty raise
- I don't use .keys() anymore
- No space before colon in English :)
- better handling of context in context['account_period_prefer_normal']
- PEP8 stuff

About my dirty lines :

    'accrued' not in context.get('type', '-') and True or False

I would prefer to use the line you suggest :

    'accrued' not in context.get('type')

but, with the above line, if there is no 'type' key in context, then it crashes (I must have a string after the 'in')

About your remark "you don't need to check if to_delete_line_ids is empty because unlink will return early if it was". You are certainly right, but I still prefer to use my "if to_delete_line_ids" because I find it more logic for the reader.

As Ronald said, using raise inside constraint checking functions is the only way to have an error message that contain relevant information inside the message. It is a technique I use very often in my code. I didn't invent it myself, I learnt it in this blog post by Albert from Nan-tic http://www.nan-tic.com/en/2010/programming-tips-constraints-and-exceptions/

-- 
https://code.launchpad.net/~akretion-team/account-closing/70-cutoff-modules/+merge/185992
Your team Account Core Editors is subscribed to branch lp:account-closing.