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.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help   : https://help.launchpad.net/ListHelp

Reply via email to