DRAFT: refactor currency converter - discussion#1331
DRAFT: refactor currency converter - discussion#1331gr4viton wants to merge 1 commit intospiral-project:mainfrom
Conversation
to allow multiple exchange rate getters defaulting to user defined csv file or hard coded exchange rates
| "ZWL", | ||
| ] | ||
| def get_currencies(self, with_no_currency: bool=True) -> list: | ||
| currencies = list(self.get_rates.keys()) |
There was a problem hiding this comment.
Or was the list of currencies hard-coded to have faster loading or something else?
I mean I removed it as I believe the currency exchange rates should be loaded close to "start-up", and then the list generated only based on the truly available ones...
There was a problem hiding this comment.
I believe we could have a set of predefined rates here, but it's really not a requirement. If we want to have some of them, we could just put the most used ones here (like USD, EUR, and a few others). We don't need the full list here though :-)
|
Hey, thanks for opening this discussion with some code attached :-) If I remember the discussion on this matter properly, I believe the way forward would be to have the ability for the users to define themselves a rate for the currency of their choosing. We could have a set of already defined currencies with set rates as a default, but the users would be able to overwrite them in the project settings page. On the approach you're proposing here, I think we wouldn't need to have a |
| raise NotImplementedError | ||
|
|
||
|
|
||
| class ApiExchangeRate(ExchangeRateGetter): |
There was a problem hiding this comment.
I believe we should ditch this way of retrieving the rates using an external API all together.
There was a problem hiding this comment.
Understandable. I saw the comment mentioning other apps are also favoring the user-defined rates - and I get the simplicty is the key and not worth maintaining part of the app which would be used just by small partition of its users (providing API keys) .
Will remove.
|
|
||
|
|
||
| class UserExchangeRate(ExchangeRateGetter): | ||
| user_csv_file = "path/to/file.csv" |
There was a problem hiding this comment.
I don't believe using a CSV file would be useful here, instead, we could propose the user to define the rates themselves for the project, in the settings.
There was a problem hiding this comment.
Understandable. Makes sense. Will try to incorporate the settings :)
| class HardCodedExchangeRate(ExchangeRateGetter): | ||
|
|
||
| def _get_rates(self) -> Optional[dict]: | ||
| return {"USD": 1.0} # TODO fill in more |
There was a problem hiding this comment.
We could have the default values defined here, for sure.
|
@gr4viton hey let me know if you need any help on this :-) |
|
FWIW, I plan on removing support for multi currencies altogether with #1362 |
2 participants
It is just a draft to discuss the approach
I will not polish this if this is not a "approved" direction - with enough simplicity in mind.
tldr
refactor currency converter
based on discussion in - #1232