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

[BugFix] Econ Calendar #6392

Merged
merged 16 commits into from
May 14, 2024
Merged

[BugFix] Econ Calendar #6392

merged 16 commits into from
May 14, 2024

Conversation

deeleeramone
Copy link
Contributor

  1. Why?:

Started as needing to add the list of countries to json_schema_extra in the TradingEconomics model, but ran into other items needing to be taken care of.

  • Standard model contained parameters and data fields that were clearly only valid for TE, the description even named 'TradingEconomics'.
  • Missing field definitions from TE response.
  • Can't lookup by country, date, and importance.
  • Literal choices not showing up in API docs.
  1. What?:

    • Remove TE-only items from the standard model.
    • Add calendar_id as a parameter for TE.
    • Can now lookup by country, date, and importance.
    • Defined the missing fields in the TE model
    • Removed Optional from Literals and they now are proper choices in the API docs.
    • "Importance" parameter made lower-case.
    • Chunk dates for FMP so that start/end dates are not bound by a 3-month window.
    • TE group parameter had one choice with a space in it.
  2. Impact:

    • @andrewkenreich, this renovates the models. Most notably, the choices for "importance" are now lower-case.
      • You will notice more fields, but I don't think you use them anyways.
    • New TE request parameter: calendar_id
    • TE country parameter now has: json_schema_extra={"choices": COUNTRIES}
    • TE requests can now do something like:
obb.economy.calendar(country="united_states", provider="tradingeconomics", start_date="2023-01-01", end_date="2023-12-31", importance="high")
  1. Testing Done:

    • Unit and integration tests.
    • Check all providers.

Note that FMP does not have any query parameters other than start/end dates.

Screenshot 2024-05-10 at 1 37 19 PM

@IgorWounds
Copy link
Contributor

@deeleeramone this looks good. For some reason I can't get tradingeconomics to work inside the CLI. Is this the same for you @deeleeramone ?

cc: @hjoaquim

@deeleeramone
Copy link
Contributor Author

For some reason I can't get tradingeconomics to work inside the CLI

cc: @hjoaquim

So in the CLI I was getting:

2 validation errors for calendar
extra_params.importance
  Input should be 'low', 'medium' or 'high' 
    For further information visit https://errors.pydantic.dev/2.7/v/literal_error
extra_params.group
  Input should be 'interest_rate', 'inflation', 'bonds', 'consumer', 'gdp', 'government', 'housing', 'labour', 'markets', 'money', 'prices', 'trade' or 'business' 
    For further information visit https://errors.pydantic.dev/2.7/v/literal_error

Adding None to the Literal for each resolved that.

@IgorWounds
Copy link
Contributor

Don't we want the Literal to be Optional? Sending a None through the API does not always work and can break.

@hjoaquim you already have some screenshots on this.

@deeleeramone deeleeramone requested a review from hjoaquim May 13, 2024 15:30
@IgorWounds IgorWounds enabled auto-merge May 13, 2024 21:13
@IgorWounds IgorWounds added this pull request to the merge queue May 14, 2024
Merged via the queue into develop with commit 17a7e7d May 14, 2024
10 checks passed
@IgorWounds IgorWounds deleted the bugfix/econ-calendar branch May 14, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change bug Fix bug platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants