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

Thanks again Sam for all this awesome code, sorry for the delay! I've
merged this and made my first pass of changes at
http://bazaar.launchpad.net/~wxbanker-devs/wxbanker/trunk/revision/870.
Mostly minor changes, except that I made "Show currency names" non-default,
otherwise that would be a big change for everyone upgrading and it wouldn't
be useful for most people using only one currency.

As you mentioned, one of your changes does change the balances to be as of
today and not total including future as they were, and since that's kind of
an unrelated change (but something I do want to improve), I'll keep it
showing the grand total for now.

A few questions for you:

   1. Now the Currency menu drop-down just changes the global currency,
   where as before it changed them all. I'm wondering if this behavior will be
   immediately understandable, or if we should prompt when you change it
   asking if you want to change the global currency, the currency for all
   accounts (since that is what it used to do and changing them all
   individually would be a pain), or the currency for the current account.
   2. What do you think should happen for new accounts? Should they be the
   localized currency by default, or the global currency?
   3. If you are able to add some tests, it would be really helpful to make
   sure all these cases going forward. Particularly, tests that handle the
   rendering with nicknames, and making sure the total balance is handled
   correctly, and such. You can run them via "python -m
   wxbanker/tests/alltests" (I think a few translation tests will fail without
   certain language packs, you can ignore those or I can let you know which
   packs are required).

Thanks again. I'm going to do a performance review (there does seem to be a
big performance regression in startup time so I need to figure that one
out), and make a few tweaks, and figure out the best way to update the
exchanges.xml, then we can release 0.9!

On Thu, Jun 7, 2012 at 12:06 AM, Sam Dieck <email address hidden> 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.
> --
>
> https://code.launchpad.net/~sam-dieck/wxbanker/per-account-currencies/+merge/108499
> You proposed lp:~sam-dieck/wxbanker/per-account-currencies for merging.
>