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

[Feature] Limit number of countries in TE request #6301

Closed
wants to merge 7 commits into from

Conversation

hjoaquim
Copy link
Contributor

  1. Why? (1-3 sentences or a bullet point list):

    • Allowing more than N countries in a TE request would break with a http code of 400 - this PR enhances the UX.
  2. What? (1-3 sentences or a bullet point list):

    • Added a validation for the number of countries in the fetcher.
  3. Impact (1-2 sentences or a bullet point list):

    • Low - small improvement on UX.
  4. Testing Done:

    • Experiment with any amount of countries from openbb_platform/providers/tradingeconomics/openbb_tradingeconomics/utils/countries.py:
from openbb import obb
countries = [...]
obb.economy.calendar(provider="tradingeconomics", country=countries).to_df()
  1. Reviewer Notes (optional):

    • It looks like it depends on the countries we're sending. Sometimes it fails for 28, other times 31/32. I wasn't able to get more than 32 with any combination of countries. Also, couldn't find any information related with this limit on TE API docs. cc @andrewkenreich

@hjoaquim hjoaquim requested a review from IgorWounds April 10, 2024 15:03
@github-actions github-actions bot added enhancement Enhancement platform OpenBB Platform v4 PRs for v4 labels Apr 10, 2024
@deeleeramone
Copy link
Contributor

deeleeramone commented Apr 10, 2024

   * Allowing more than N countries in a TE request would break with a http code of 400 - this PR enhances the UX.
   * It looks like it depends on the countries we're sending. Sometimes it fails for 28, other times 31/32. I wasn't able to get more than 32 with any combination of countries. Also, couldn't find any information related with this limit on TE API docs. cc @andrewkenreich

Is there a reason why "All" is not used instead of trying to get a huge list? It could quite easily create a URL string that is too long for HTTP requests.

It would be pretty simple to filter post-request, and storing that data in cache would dramatically reduce the number of API calls.

@hjoaquim
Copy link
Contributor Author

   * Allowing more than N countries in a TE request would break with a http code of 400 - this PR enhances the UX.
   * It looks like it depends on the countries we're sending. Sometimes it fails for 28, other times 31/32. I wasn't able to get more than 32 with any combination of countries. Also, couldn't find any information related with this limit on TE API docs. cc @andrewkenreich

Is there a reason why "All" is not used instead of trying to get a huge list? It could quite easily create a URL string that is too long for HTTP requests.

It would be pretty simple to filter post-request, and storing that data in cache would dramatically reduce the number of API calls.

Is all even an option? Couldn't find that on TE API docs.
I like the idea, if possible.

Although, we can still set a limit for number of countries in one request, if TE has one (as it look it does), using the validator.

@IgorWounds
Copy link
Contributor

@deeleeramone

@deeleeramone
Copy link
Contributor

Is all even an option? Couldn't find that on TE API docs. I like the idea, if possible.

Although, we can still set a limit for number of countries in one request, if TE has one (as it look it does), using the validator.

All is what you get when you only enter a start/end date. A different way of going about restricting the length of the URL would be by the character count instead of the number of countries.

2000 characters is a general "safe" limit for URL length, although this can vary by platform.

@hjoaquim
Copy link
Contributor Author

Is all even an option? Couldn't find that on TE API docs. I like the idea, if possible.
Although, we can still set a limit for number of countries in one request, if TE has one (as it look it does), using the validator.

All is what you get when you only enter a start/end date. A different way of going about restricting the length of the URL would be by the character count instead of the number of countries.

2000 characters is a general "safe" limit for URL length, although this can vary by platform.

That seems to be the case - let's do that instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants