Code review comment for lp:~flimm/wxbanker/none-bugfix

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

This is pretty reasonable, though I'm worried about what is causing it in the first place, as I'd not like to ignore that. However, it still seems fine to defend against. Instead of the ternary though, it is probably best to just use a simple expression like "self.Currency = currency or 0" for each.

I will happily merge this if you wouldn't mind just:
 * switching them to just using 'or'
 * add a comment above briefly mentioning why (to defend against None) and mentioned the LP #
 * add a simple test in tests/modeltests.py that creates an account by passing None as currency and balance and makes sure the object ends up with 0 and 0.0

Let me know if you need any tips or help! The comment will make it clear to anyone else looking at it later why the logic is there, and the test will make sure there isn't a regression later. It probably isn't obvious enough how to run the tests, but from the root run "python -m wxbanker/runtests". To test you won't be able to just model.CreateAccount, but instead import Account from wxbanker.bankobjects and directly instantiate.

review: Needs Fixing

« Back to merge proposal