Code review comment for lp:~camptocamp/openobject-addons/trunk_webkit_improvment_rc

Hello,

For the try catch block around the render method I strictly followed the mako doc. That why there is not one unique try except block but one around each render call. Mako does not provide a render exception type (or I missed it in doc). The exception returned is munch more easy to understand. But I agree with the osv_except point and the general Exception type catch point (I have to test if it works).

For the margin I prefer to keep float because setting margin in different unit may crash wkhtmltopdf, and it prevent wrong entries. But why not provide a select field with supported unit and set mm as default.

Many thanks

Nicolas

Le 17 nov. 2010 à 16:53, "Olivier Dony (OpenERP)" <email address hidden> a écrit :

> Review: Needs Fixing
> Hello Nicolas,
>
> A few remarks:
> - adding try/except blocks without specifying the exception class is a no-go, you need to at least specify "except Exception" if you don't know a more precise class to catch. Also, couldn't it be a single try/except block?
>
> I haven't checked, but do you know if the "Exception" type that you are throwing is properly reported to the user, or is it simply displayed in the log? Don't you want to use an except_osv one?
> I know I had to fix several things during the initial review to get exceptions reported to the user correctly.
>
> - As for the question about 0 margins, OpenERP does not yet support distinguishing 0 from NULL values for integers. However I wonder if your margins fields should not be fields.char instead, because the wkhtmltopdf params are strings indeed, as you can pass "10px" or "10.5cm", etc. This would give you more flexibility, wouldn't it? Note: You need to be careful however because I think wkhtmltopdf only supports the same unit of measure for all margins, and the default ones are "10mm". So when specifying a unit it needs to be set for all margins and it must be the same.
>
> I'm okay with the latter change for RC2, as it is needed to properly fix the margins.
>
> What do you think?
> --
> https://code.launchpad.net/~c2c/openobject-addons/trunk_webkit_improvment_rc/+merge/40847
> Your team Camptocamp is subscribed to branch lp:~c2c/openobject-addons/trunk_webkit_improvment_rc.