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 ECBrates source for exchange rates #82

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mrlimacz
Copy link
Contributor

@mrlimacz mrlimacz commented Oct 12, 2023

This source used ECB datasets directly (daily average spot rates). As they only provide EUR as the base currency, other pairs need to be calculated as XXX -> EUR -> YYY

@gpoul
Copy link

gpoul commented Jan 7, 2024

Sounds like a good idea; I just tested this on my end and I'm getting empty response from the ECB API you're calling. I added the following check:

    if len(response.text) == 0:
        raise ECBRatesError(f"Empty response text received")

@gpoul
Copy link

gpoul commented Jan 7, 2024

Reason seems to be that I ran this on a day without data available (Sunday) and it takes the current date by default:

➜  ~ http GET https://data-api.ecb.europa.eu/service/data/EXR/D.USD.EUR.SP00.A startPeriod==2024-01-07 endPeriod==2024-01-07 format==jsondata detail==full --verbose
GET /service/data/EXR/D.USD.EUR.SP00.A?startPeriod=2024-01-07&endPeriod=2024-01-07&format=jsondata&detail=full HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: data-api.ecb.europa.eu
User-Agent: HTTPie/3.2.2



HTTP/1.1 200 
Cache-Control: max-age=30
Connection: keep-alive
Content-Disposition: attachment;filename=data.json
Content-Length: 0
Content-Type: application/vnd.sdmx.data+json;version=1.0.0-wd
Date: Sun, 07 Jan 2024 13:00:17 GMT
Expires: Sun, 07 Jan 2024 12:54:18 GMT
Last-Modified: Fri, 05 Jan 2024 14:57:01 GMT
Server: myracloud
Vary: accept


@mrlimacz
Copy link
Contributor Author

@gpoul Actually, such instances are handled. When you run bean-price with an --update flag you want to skip a particular date and proceed to the next one. Your approach would derail the whole thing, ECBRatesError would finish the execution.

@gpoul
Copy link

gpoul commented Jan 27, 2024

I can only say that I get this error when running it unmodified from your branch:

(beancount-fava) ➜  financial git:(master) ✗ bean-price -e EUR:ecbrates/EUR-USD --update
ERROR   : Could not fetch for job: DatedPrice(base='EUR-USD', quote='EUR', date=None, sources=[PriceSource(module=<module 'beanprice.sources.ecbrates' from '/Users/gpoul/Projects/financial/beancount-fava/lib/python3.11/site-packages/beanprice/sources/ecbrates.py'>, symbol='EUR-USD', invert=False)])

If I supply a date that returns data, then yes; it works:

(beancount-fava) ➜  financial git:(master) ✗ bean-price -e EUR:ecbrates/EUR-USD -d 2024-01-26
2024-01-26 price EUR-USD                            1.0871 EUR

My expectation was that if there is no most recent price it should return the latest price it can fetch including its date, no? That's at least how I saw other modules behave so far, but maybe I'm misunderstanding how it works.

@blais
Copy link
Member

blais commented Jun 16, 2024

Please add a test.
In adding test, be mindful of people running them in different timezones.
Thank you,

@mrlimacz
Copy link
Contributor Author

mrlimacz commented Jan 4, 2025

As @gpoul suggested a year ago (sorry for that), I made an adjustment which gives a latest available rate (instead of a dedicated exception) when there is no rate for a given date (weekends, holidays).

@blais I also added a unit test. Pylint check fails due to lines too long in ecbrates_test.py, but these are API responses needed for tests. Other errors come from eastmoneyfund_test.py, not a subject to this PR.

@blais
Copy link
Member

blais commented Jan 6, 2025

Please fix the linter errors and I'll rubberstamp this.

@mrlimacz
Copy link
Contributor Author

mrlimacz commented Jan 6, 2025

@blais I fixed linter errors in my test, but it still fails on eastmoneyfund_test.py, which does not exist on my branch

@mrlimacz
Copy link
Contributor Author

mrlimacz commented Jan 6, 2025

@blais I created PR #93 which fixes eastmoneyfund_test.py, though it fails another check.

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.

3 participants