Code review comment for lp:~elbati/hr-timesheet/adding_hr_attendance_analysis_7

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

I admit that I just did a quick eyeballing on the whole merge proposal, scrolling over some parts.

The feature seems very nice.

Here are some observations which are more nitpickings than real issues:

* Why is there a dependency on python 2.7?

l.146 from openerp.tools.translate instead of from tools.

l.162, what does means this 0.01666667 ?
 - the unit worth be precised in the `working_time_precision` field
 - 1.0 / 60 is more readable than 0.0166...

l.221, typo in the precision I think

l.227 the timedelta can be far easier to read with keyword arguments (timedelta(hours=precision)

l.268 using \ is to avoid, even more in arithmetic operations I think, just use the continuation inside parenthesis if the line is too long

    minutes = (datetime.minute / 60.0 +
               datetime.second / 60.0 / 60.0)

l.1937-1983 using dicts would be appropriate here instead if many elif

« Back to merge proposal