alexprengere / currencyconverter

A Python currency converter using the European Central Bank data.
http://alexprengere.github.io/currencyconverter
Apache License 2.0
215 stars 59 forks source link

Documentation of CurrencyConverter #6

Closed thorbjornwolf closed 6 years ago

thorbjornwolf commented 6 years ago

Hi! Thanks for doing all this work :-)

Now, I've been looking through the code a number of times, but I'm still fuzzy on the exact roles of __init__'s fallback_on_wrong_date and fallback_on_missing_rate. Extrapolating this issue, it might be beneficial to document all the arguments for at least __init__ and convert, since the end user will need both to use this package.

If the maintainer is interested in receiving a PR, I wouldn't mind giving it a shot.

My understanding so far of __init__'s arguments:

Is that about right? :-)

alexprengere commented 6 years ago

Hi, Thanks for taking the time to dig in the code and write this. I thought the examples were clear but evidently my judgement was completely biased 😉.

You are right on currency_file, fallback_on_wrong_date, ref_currency, verbose and na_values.

Regarding fallback_on_missing_rate, you are almost right, indeed the data loading process will fill the holes inside the currency data's date range, but it will use a linear interpolation using the two closest rates available (appropriate test). I agree that this is not obvious by reading the code, but it was written this way to keep the complexity linear when filling the holes.

Feel free to send a PR, otherwise I can just update the README with more details in the fallback section.

thorbjornwolf commented 6 years ago

Ahhh, I see! And what a blunder I have made - I've solely been looking at docstrings (and falling back to source code) to understand the arguments, not even considering the README :disappointed:

It might be that others are like me, using CurrencyConverter? in iPython as the main way of inspecting documentation, so I'll make a PR to show you what I was expecting. Then we'll see if it makes sense to include in the package.

thorbjornwolf commented 6 years ago

Fixed with #7