Frankfurter update #166
Frankfurter update #166
Conversation
ingestr/src/frankfurter/helpers.py
Outdated
| @@ -19,6 +19,11 @@ def get_path_with_retry(path: str) -> StrAny: | |||
| def validate_dates(start_date: datetime, end_date: datetime) -> None: | |||
| current_date = pendulum.now() | |||
|
|
|||
| print("START DATE") | |||
There was a problem hiding this comment.
we can remove these debug print statements
ingestr/main_test.py
Outdated
| dest_table, | ||
| ) | ||
| assert result.exit_code != 0 | ||
| assert has_exception(result.exception, ValueError) |
There was a problem hiding this comment.
This should be ingestr.src.errors.UnsupportedResourceError
ingestr/main_test.py
Outdated
| def interval_start_does_not_exceed_current_date(dest_uri): | ||
| schema = f"testschema_frankfurter_{get_random_string(5)}" | ||
| dest_table = f"{schema}.frankfurter_{get_random_string(5)}" | ||
| start_date = pendulum.now().add(days=1).format("YYYY-MM-DD") | ||
| result = invoke_ingest_command( | ||
| "frankfurter://", | ||
| "exchange_rates", | ||
| dest_uri, | ||
| dest_table, | ||
| interval_start=start_date, | ||
| ) | ||
| assert result.exit_code != 0 | ||
| assert has_exception(result.exception, ValueError) | ||
| assert "Interval-start cannot be in the future." in str(result.exception) | ||
|
|
||
|
|
||
| def interval_end_does_not_exceed_current_date(dest_uri): | ||
| schema = f"testschema_frankfurter_{get_random_string(5)}" | ||
| dest_table = f"{schema}.frankfurter_{get_random_string(5)}" | ||
| start_date = pendulum.now().subtract(days=1).format("YYYY-MM-DD") | ||
| end_date = pendulum.now().add(days=1).format("YYYY-MM-DD") | ||
| result = invoke_ingest_command( | ||
| "frankfurter://", | ||
| "exchange_rates", | ||
| dest_uri, | ||
| dest_table, | ||
| interval_start=start_date, | ||
| interval_end=end_date, | ||
| ) |
There was a problem hiding this comment.
Although I like the idea of these tests, it can cause subtle issues for our users.
For instance, depending on which timezone frankfurter uses, the interval_end parameter may be valid.
There was a problem hiding this comment.
solutions:
- Remove --interval-end checks exceeding the current date. Users are responsible for providing the correct parameters.
-- or -- - Figure out what timezone frankfurter uses and translate all date/time inputs to that timezone and then run validation on end date.
| @@ -41,7 +41,9 @@ ingestr ingest \ | |||
| ### **`--interval-end` (Optional)** | |||
| - **Description**: The end date for fetching historical exchange rates. | |||
| - **Value**: A date in the format `YYYY-MM-DD` (e.g., `'2025-03-28'`). | |||
| - **Purpose**: Defines the ending point for fetching historical data. If not provided, it defaults to the value of `--interval-start`. | |||
| - **Purpose**: Defines the ending point for fetching historical data. | |||
| - If not provided, it defaults to the value of `--interval-start`. | |||
There was a problem hiding this comment.
--interval-end should be the current date. In fact, frankfurter treats a missing end date as the latest.
There was a problem hiding this comment.
Yeah, this was due to a design decision. Sabri suggested having a single historical date as part of exchange rates, as one is a subset of the other. I therefore diverged slightly from the API.
The easiest solution to this problem would be to require an end date or throw an exception.
A more involved solution would be to create follow the API and default to latest and to implement a table for a single historical date and call it something like historical_date. It would probably be a more elegant solution. I started the implementation and then realised I might just be creating more review work for you and more testing work for me.
What do you think?
There was a problem hiding this comment.
I see where you are coming from.
I can think of a few scenario's from a user's prespective:
- they need the latest exchange rate to covert currencies for online-transactions.
- they want to keep a historical record of currency rates
In case (1) the user will anyway use the latest resource. For (2) the user would probably want to have upto-date data from a specific starting point.
Can you think of any scenario where the user would want only a certain day's exchange rate? And if so, should that be the default expectation with everyone?
Adding the following for Frankfurter: