openerp-dev-web team mailing list archive

["Xavier \(Open ERP\)" <xmo@xxxxxxxxxxx>]

Review: Needs Fixing
0. Please perform reformattings (even automated by your IDE) in different revisions than you perform actual work, mixing them makes reviews and reading history harder than it should (you are not limited in the number of revisions you use, so don't worry about that)

1. If we have only one value (namely CSV), there is no need to display the control/section at all since there is no choice and csv is the default output format. Thus instead of a sequence of export format you could just as well use a boolean `enable_excel_export` (or something like that) flag and only display the control if it's `True` (as far as I know, we're not expected to add new export formats anytime soon). That flag would simply be set if we can import xlwt, unset otherwise

2. This means you can also move the xlwt import to the toplevel of the module (instead of within functions) wrapped in an except

3. Please use precise as exception catch as you can: the error we're looking for here is specifically ImportError, we don't want to "eat" exceptions due to e.g. a broken xlwt installation
-- 
https://code.launchpad.net/~openerp-dev/openobject-client-web/export/+merge/41588
Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-client-web/export.