Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement forex python #225

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

macanudo527
Copy link
Collaborator

@macanudo527 macanudo527 commented Mar 4, 2024

What?

Fixes #224
Dependent on #223
This allows users to make use of CSV files for Forex pricing instead of rely on exchangerate.host

Testing

I have not tested this with real world data at all. This is curently WIP.

@macanudo527
Copy link
Collaborator Author

@eprbell this reads in missing forex data from CSV files. But, I did finally find this API that can basically do the same thing, but via a REST API. Should I keep both and use the CSV files as a fallback?

@eprbell
Copy link
Owner

eprbell commented Mar 22, 2024

@eprbell this reads in missing forex data from CSV files. But, I did finally find this API that can basically do the same thing, but via a REST API. Should I keep both and use the CSV files as a fallback?

Sounds good, but should it be a separate plugin, so that the user can decide which to use and with what priority?

@macanudo527
Copy link
Collaborator Author

Sounds good, but should it be a separate plugin, so that the user can decide which to use and with what priority?

I'm wondering about how to implement that. Maybe have two different CCXT pair converters one with the REST API, and one with the importing of CSV?

@eprbell
Copy link
Owner

eprbell commented Mar 25, 2024

Sounds good, but should it be a separate plugin, so that the user can decide which to use and with what priority?

I'm wondering about how to implement that. Maybe have two different CCXT pair converters one with the REST API, and one with the importing of CSV?

Yes, that's what I was thinking: that should give the user the most flexibility.

@macanudo527
Copy link
Collaborator Author

@eprbell btw, I'm going to try to cherry pick and splice the code from your PR. Hopefully, I can get it all put together here soon.

Should I implement this with inheritance, or by parameter that will load a particular class to handle forex processing? I'm thinking making ccxt pair converter abstract and the forex processing has to inherit from it. OR, make the REST API default, and then have another CSV class inherit and override the defaults.

@eprbell
Copy link
Owner

eprbell commented Mar 26, 2024

@eprbell btw, I'm going to try to cherry pick and splice the code from your PR. Hopefully, I can get it all put together here soon.

Sounds good, no worries.

Should I implement this with inheritance, or by parameter that will load a particular class to handle forex processing? I'm thinking making ccxt pair converter abstract and the forex processing has to inherit from it. OR, make the REST API default, and then have another CSV class inherit and override the defaults.

I'm leaning toward making the ccxt-based converter abstract and have the two forex processing plugins inherit from it. In theory we could do something similar to the input plugins and have csv and rest subdirectories, but that would not be backward compatible, would require changes in users config files, and would make this change more complex: let's leave it as is. If needed, we'll tackle that issue as a separate change later.

@macanudo527 macanudo527 force-pushed the implement_forex_python branch 6 times, most recently from 8b754be to 9a57efb Compare April 17, 2024 00:43
@macanudo527 macanudo527 force-pushed the implement_forex_python branch 2 times, most recently from 0b2ea73 to 8aebb3c Compare April 19, 2024 00:48
@macanudo527 macanudo527 force-pushed the implement_forex_python branch 6 times, most recently from 6286ef4 to 6414ff7 Compare April 27, 2024 00:21
@macanudo527 macanudo527 mentioned this pull request May 13, 2024
@macanudo527 macanudo527 marked this pull request as ready for review May 13, 2024 05:29
@macanudo527
Copy link
Collaborator Author

@eprbell This should fix most Forex Issues. It is lacking docs, but I thought I would let you get a peek at it.

@eprbell
Copy link
Owner

eprbell commented May 13, 2024

@eprbell This should fix most Forex Issues. It is lacking docs, but I thought I would let you get a peek at it.

Sounds good: I'll take a look (the diff in Github is large so it may take some time for me to go through it).

@macanudo527
Copy link
Collaborator Author

macanudo527 commented May 13, 2024

Sounds good: I'll take a look (the diff in Github is large so it may take some time for me to go through it).

I figured it would take a while. I tried to figure out a way to break down into simpler parts, but kind of just had to be done all at once. Sorry.

@macanudo527 macanudo527 changed the title [WIP] Implement forex python Implement forex python May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forex Services Unavailable
2 participants