Code review comment for lp:~kolmis/wxbanker/transaction-tagging

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

I'll propose this for a merge since it would be nice to integrate, and then I can potentially merge the categories branch as well.

One issue I noticed off-hand is that using a list as a default argument probably isn't what you want (tags=[]) as default arguments are only evaluated once thus it will be the same list:

>>> def foo(x=[]):
... x.append(1)
... print x
...
>>> foo()
[1]
>>> foo()
[1, 1]
>>> foo()
[1, 1, 1]

So typically in this case what you do is set tags=None and at the top of the function say if tags is None: tags = []

What are your thoughts on merging this branch? I don't want an extra tags column by default as I want wxBanker to be as simple and clean as possible as surely some users won't need tags, but I'd like it to be as discoverable as possible. Perhaps have some exposed way to tag a transaction, and then only show the column for accounts which have a tagged transaction.

« Back to merge proposal