Use Unified CSV #223
Use Unified CSV #223
Conversation
macanudo527
force-pushed
the
use_unified_csv
branch
2 times, most recently
from
7b63e51
to
2ea8c47
Compare
macanudo527
force-pushed
the
use_unified_csv
branch
from
2ea8c47
to
0b668f9
Compare
|
Unfortunately I can't test this. Exchangerate.host has restricted their free tier to just 100 calls a month! Definitely need to find a better solution. There are plenty of places that allow you to download daily data for free in CSV form, but you need to register and download. User will need to manually download the CSV and then we can chunk it. |
Oh, no, what a pain... Does the fact that you can't test it mean that we need to abandon this PR? If there's no better alternative, I think it's OK to ask users to download the CSV as needed. |
This PR should be okay. It worked before I did a code clean. I just wanted a final test after the code clean to verify everything. The free tier offers up 100 requests which might be adequate for people for now. That would include about 90 or so days of trades. I think for a normal user that will be kind of okay. I just run bots that are constantly trading, so I need every day of rates. |
|
@eprbell We should have just use this from the beginning. -> forex-python. I'm not sure why it didn't come up before. |
|
OK, I'll review it then. Also I took a quick look at forex-python: it does look like what we need! |
|
@eprbell just an FYI. When I create [WIP] commit, I'll later force push a change to finish it up. I basically code on two different PCs and so I use Github to transfer changes between the two. Sometimes I can't completely finish a commit though. |
macanudo527
force-pushed
the
use_unified_csv
branch
from
d79f988
to
a70cef3
Compare
|
@eprbell I'm not sure why the tests are failing. Pylint seems to have a problem with stubs. Otherwise, this should be ready to go. |
|
@eprbell do I need to do more for this? I'm not getting these errors locally. |
This may be a bug in the latest release of pylint (https://github.com/pylint-dev/pylint/releases): it may be worth trying again after fixing the release to 3.0.4 or 3.0.3. |
I set it to ignore all .pyi files. We were basically doing that before by declaring the directories in the cmdline. |
Co-authored-by: eprbell <77937475+eprbell@users.noreply.github.com>
macanudo527
force-pushed
the
use_unified_csv
branch
from
90ed300
to
38d75c8
Compare
|
@eprbell Is there something special I need to do for the document check? That Kraken link isn't dead. |
That sometimes happen in Github CI (for mysterious reasons): the solution is to enclose the link in |
|
@eprbell is it possible to get this approved? |
Yes, I'll do one more full review in the weekend. I've been snowed under at work and didn't have much time. |
|
@eprbell I removed the unified CSV id. |
|
Sounds good: as soon as other comments are addressed, I'll review one more time. |
@eprbell Oops, I committed the clarify comments commit to the forex PR. I cherry picked it to this one. |
This addresses several bugs. I tried to each part in a separate commit so that it is easy to review.
It basically fixes the problem with Kraken CSV files being put in one big CSV file.
I have to do a full pull to test it again. Then, I can fix the commented out parts in the CCXT pair plugin.