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

Revision history for this message
Sam Dieck (samd) wrote :

* Namespaces are now corrected and not using * when importing.

* About the use of the .Balance attribute, i think that the code in question is the following:

            def GetBalance(self):
    208 - return sum([account.Balance for account in self])
    209 + totalCurrency = self.Store.getGlobalCurrency()
    210 + total = 0
    211 + for account in self:
    212 + total = total + account.GetCurrentBalance(totalCurrency)
    213 + return total

  Here are my thoughts on this code, and hopefully you could clarify me on this. From what I see in the function "GetCurrentBalance" the attribute ".Balance" contains even future transactions, and that's the difference between using "GetCurrentBalance and the attribute "Balance" (actually, even GetCurrentBalance() makes use of .Balance) ; do we want to include future transactions on total balances? even if those transactions have not been made? If that's the case, its just matter of creating a new function ".GetBalance(currency)" in the account class which returns the ".Balance" property converted to the desired currency without substracting future transactions (as GetCurrentBalance() does).

* I would be more than happy to run/add more tests, is there anything i need to know before going into that?

* Alright, i got ya on the exchanges.xml, ill start implementing it and tell you if i run into any problems and need help. Dont you think that it'll be better to add a "currency rates settings" where you can see current rates, click on an "update" button to update current rates, and possibly add an option for "check for currency updates automatically on startup" as i don't think that blindly pulling things from the internet without even a notification to the user is a good idea.

« Back to merge proposal