Code review comment for lp:~samd/wxbanker/per-account-currencies

Revision history for this message
Michael Rooney (mrooney) wrote :

Thanks for this awesome code, overall it looks pretty great! A few comments:

* it would be great to remove usages of "from wxbanker.currconvert import *", for an easier to follow namespace. If you don't want to have to type the full path each time, you could use "from wxbanker import currconvert" and then use it that way.
* is it no longer using Account.Balance, but the GetCurrentBalance function? I'm just wondering if this defeats the point of caching this value and now has to add up all transactions for all accounts to display the totals. If so, maybe it could just convert the cached .Balance attribute, but if I'm misunderstanding, let me know.
* it would be great to add a few tests (there are bunch located at wxbanker/tests/ including UI tests)! I think I did a bad job at documenting how to run them so I'll attempt to get them running with nose and a simple "setup.py test" or something.
* regarding your exchanges.xml question, the idea was the ship each version with an up-to-date (at the time) version for offline use, but on startup grab the latest data if online (from http://www.ecb.europa.eu/stats/eurofxref/eurofxref-daily.xml) and write it back to exchanges.xml. I can help implement this feature if you'd like.

review: Needs Information

« Back to merge proposal