carlospalol / money

Python money class with optional CLDR-backed locale-aware formatting and an extensible currency exchange solution.
MIT License
230 stars 34 forks source link

0 dollars is the same as 0 euros.. or anything. #23

Closed g-- closed 7 years ago

g-- commented 7 years ago

Allow comparisons between Money and 0 (integer, decimal, or otherwise). This shortens code from:

this._ZERO = Money(0, this.currency)
if x > this._ZERO:
...

to:

if x > 0:
...
carlospalol commented 7 years ago

Hi Geoff, it is more or less the same, i.e. if you don't care about inadvertently mixing currencies. This change was introduced in 1.3.0 with the suggestion of using m.amount > 0 for a usecase like yours. Have you tried that approach? Is it not working for you? Could you explain?

Note: just changing the CHANGELOG entry of a previously released version would defeat the purpose of the changelog.

g-- commented 7 years ago

Hi Carlos!

Thanks for reviewing my PR!

First, my mistake with the changelog: I thought 1.3.0 was a yet-to-be-released version.

I see your point about m.amount > 0 being about as easy as m > 0.

My thought process was: do I just want syntactic sugar, or does it logically make sense to compare a currency with a naked 0 without breaking the level of abstraction? Take the example:

def format_amount(m): if m < 0: Return "({})".format(m) Else: Return str(m)

with 0 recognized as money, this code is completely agnostic of the money type. M could be a decimal or integer instead of money and it would work fine.

I think it also looks cleaner:

Total = invoice_items - credits - deposit - payments - discounts If Total < 0

Post a customer credit

Else:

Send invoice as normal

But could it introduce bugs? Possibly if someone was mixing the money and non money types and their tests only produced zero for the naked numbers even if others were possible. If we could restrict it to literal zeros, that would be helpful.

What do you think?

Geoff

carlospalol commented 7 years ago

I understand your perspective (it was after all the original implementation), and I totally agree m > 0 looks nicer, but it is an exceptional case. It was very inconsistent with >=, <=, and ==.

It is clearer and safer to either care about the currency (m1 > m2) or not (m.amount > x).

For your first example, I think you should use m.format() or indeed adapt your code to check the sign with m.amount > 0.

carlospalol commented 7 years ago

@ g--, I'm closing this for now. Thanks for raising the issue, I need to make a better job explaining in the readme the rationale of how it works now.