← Back to team overview

openerp-dev-web team mailing list archive

Re: [Merge] lp:~openerp-dev/openobject-client-web/export into lp:openobject-client-web

 

> 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

Conclusion what I understood: If xlwt is not installed then only CSV option will be there and as boolean option "Excel" option will be available. When user will click for Excel then error message will raise "Please Install xlwt Library ......". RIGHT ??
 
> 2. This means you can also move the xlwt import to the toplevel of the module
> (instead of within functions) wrapped in an except

If we import xlwt to the top level, then how will we check that it is installed or not ?
How can we raise error ?
We have to keep in try/catch somewhere.

> 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.



References