openerp-community-reviewer team mailing list archive

[Daniel Reis <dgreis@xxxxxxx>]

Thanks for the thorough review, Sandy.
I just pushed the fixes, and here go some comments on it:

> PEP8 issues:

You  just made me realize my pep8 configs needed some tuning.
E123 is not strict PEP8 (see E123 on https://pep8.readthedocs.org/en/latest/intro.html), and I choose not to follow it: I feel that it makes you add unnecessary indentation or extra lines.
The E241 were fixed.

> Code issues:
> project_sla/m2m.py:58:12: redundant parenthesis, if you want to make it a
> tuple, it should be [(5, )]

You are right. I ended up using [(5, 0)] because I found code in the standard addons using it like that.

> context is None not handled (if context is None: context = dict())

If context is not manipulated and is only passed through, it's unnecessary to handle the context == None case. So, I didn't fix these.

 
> Minor code issues:
> project_sla/project_sla_control.py:112:15: would have gone with: now =
> dt.now().strftime(DT_FMT)
> project_sla/project_issue_view.xml:60:1: Commented out code
> project_sla/project_sla.py:24:1: no _description
> project_sla/project_sla.py:65:1: no _description
> project_sla/analytic_account.py:23:1: _logger declared but never used

Yes; fixed.

> project_sla/project_sla_control.py:104:17: Xml Tag Has Empty Body
> project_sla/project_sla_control.py:104:17: Xml Tag Has Empty Body
> project_sla/project_sla_control.py:65:20: _description not translated
> project_sla/project_sla_control.py:292:20: _description not translated

Hmm, didn't find XML tags on those .py files, and I found the translation strings in the .pot file. Can you check this again?
 
> Spelling:

Fixed.

> Out of curiosity, I noticed that you're using CamelCase for your class names,
> is there a reason for that?

I'm using it because [PEP8](http://www.python.org/dev/peps/pep-0008/#class-names) states that "class names should normally use the CapWords convention", and I don't any reason to not follow it.

-- 
https://code.launchpad.net/~dreis-pt/contract-management/7.0-project_sla-dr/+merge/199645
Your team OpenERP Community Reviewer/Maintainer is subscribed to branch lp:contract-management.