convert_money should return money instance?

[tzenderman]

Hey!

Very nice work on this repo, I'm integrating it now to my project and want to contribute back to the repo soon as well.

With regards to the djmoney_rates.utils.convert_money function, is there a reason it returns a Decimal and not a Money instance? I can always create another util that will return the money instance the way I am thinking, but let me know.

I will try to add some basic installation info to the Readme soon, is that ok?

-Tim

[synasius]

Hi @tzenderman

Very nice work on this repo, I'm integrating it now to my project and want to contribute back to the repo soon as well.

Thnx! I'll love to have your contributions :)

With regards to the djmoney_rates.utils.convert_money function, is there a reason it returns a Decimal and not a Money instance? I can always create another util that will return the money instance the way I am thinking, but let me know.

The point here is that I'd like to have two functions: a vanilla convert_money that returns decimal and a specialized function that outputs Money instances. The current convert_money is a base that one can easily wraps, but we should definitely add the specialized version that works with money instances. something like:

def convert_money(money, currency_to):
    # money should be an instance of moneyed.Money
    amount = base_convert_money(money.amount, money.currency.code, currency_to)
    return moneyed.Money(amount, currency_to)

What do you think? Does it work?

I will try to add some basic installation info to the Readme soon, is that ok?

Great! thnx! Also add yourself to contributors in AUTHORS.rst

[tzenderman]

Awesome!

With respect to the converting money functions, that's exactly what I was thinking. I wasn't sure if you had already wired convert_money into some projects and would break if we made it return a money instance now though. base_convert_money sounds right and I think the convert_money works correctly too. I'll fork, try it out and PR it back soon.

In the project we're using this in, we're updating exchange rates with a celery period_task once a day. I'll see how I can add that as a setting in the repo too as I think it would be super useful...

[synasius]

I'll fork, try it out and PR it back soon.

:+1:

In the project we're using this in, we're updating exchange rates with a celery period_task once a day. I'll see how I can add that as a setting in the repo too as I think it would be super useful...

Yep, would be nice to have such a feature! Keep in mind to put this code in a separate PR. that way it's easier for me to review.

[synasius]

this is solved in #3