openerp-community-reviewer team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

Review: Approve

Works fine, thanks! I agree with the other changes (reference to a specific service in a generic log msg, and have the most recent log on top).

About the feedparser dependency: while it is a dependency in OpenERP's setup.py, it is not a core library for OpenERP and may drop away in future versions. This may be easily missed during the upgrade of this module at that time. In fact, it is only used to guess encodings of emails in a piece of foreign code (by the late Aaron Schwartz no less). And in this code, a missing feedparser is caught by ImportError with a default to 'utf-8' so you can actually totally get away with running OpenERP without feedparser. But because like what has been said, only a part of the users of this module will check this particular service so you don't want to force an additional dependency on the other users. Fortunately, an ImportError because of feedparser will not crash the OpenERP server in this case, because of this module's strategy when running a service. instead, the error is logged in the update log for the user to see, which is perfectly acceptible.





-- 
https://code.launchpad.net/~oerp.ca/account-financial-tools/add-bank-of-canada-rss/+merge/204377
Your team OpenERP Community Reviewer/Maintainer is subscribed to branch lp:account-financial-tools.