openerp-community-reviewer team mailing list archive

["Matthieu Dietrich @ camptocamp" <matthieu.dietrich@xxxxxxxxxxxxxx>]

Review: Disapprove

> > Some minor nitpicks:
> >
> > L9: why import osv? I think you can "orm.except_orm()".
> > L22: I can see it was already there, but maybe you can improve the message
> > (what does "(resp. Sign out)" mean?)
> > L38,L51: isn't the comment over indented?
> > L80: turn that comment into a docstring (""" explanation """)
> >
> > You might also check PEP8; at least line 92 is not compliant.
> 
> Thanks for the feedback. Actually, I realize I merged another branch by
> mistake, and those are most of the nitpicks.
> I'll probably re-do the whole branch properly, and modify line 92 to be
> PEP8-compliant.

Actually, checking the structure more thoroughly, the _defaults is unnecessary.
The department_id on the hr.analytic.timesheet comes from the _inherits of account.analytic.line, and this one already has a _defaults.

This means that this whole branch is unneeded. I'll just remove it.
-- 
https://code.launchpad.net/~camptocamp/hr-timesheet/7.0-timesheet-add-department-by-default/+merge/195965
Your team OpenERP Community Reviewer/Maintainer is subscribed to branch lp:hr-timesheet.