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

hotfix/Frontend sync #6327

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open

hotfix/Frontend sync #6327

wants to merge 22 commits into from

Conversation

tehcoderer
Copy link
Contributor

@tehcoderer tehcoderer commented Apr 19, 2024

Frontend sync

@github-actions github-actions bot added bug Fix bug platform OpenBB Platform v4 PRs for v4 breaking_change labels Apr 19, 2024
@tehcoderer tehcoderer marked this pull request as ready for review April 19, 2024 21:44
@@ -144,3 +149,27 @@ def get_interval(value: str) -> str:
}

return f"{value[:-1]}{intervals[value[-1]]}"


# some fmp endpoint return date in EST without a timezone, this function will parse it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They actually return the date in the timezone of the exchange, without a timezone. For example:

https://financialmodelingprep.com/api/v3/historical-chart/5min/MNDI.L?

  {
    "date": "2024-04-08 08:05:00",
    "open": 1400.5,
    "low": 1396.5,
    "high": 1400.5,
    "close": 1398,
    "volume": 5298
  },
  {
    "date": "2024-04-08 08:00:00",
    "open": 1395,
    "low": 1395,
    "high": 1407.5,
    "close": 1399,
    "volume": 4883
  }

The London Stock Exchange is open Monday through Friday from 8:00 am to 4:30 pm British Summer Time (GMT+01:00).

You are localizing that as 8:00 AM EST, which is not a correct TZ conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smh they really are the best 💀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the reason why it's not localized, it's better to leave it alone than to make it incorrectly tz-aware.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then I need to figure out the current timezone per symbol, or else all dates will be treated as UTC on the frontend. RIP

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like something frontend needs to deal with independently of the fetcher. These should stay tz-unaware from the fetcher.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeleeramone if we have to do this on FE can you get us a list of all exchanges and the time zone they in

Ty

Copy link
Contributor

@deeleeramone deeleeramone Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from FMP directly, at least AFAICT. They also don't use the ISO codes for exchanges.

https://github.com/gerrymanoim/exchange_calendars

return [FMPFinancialRatiosData.model_validate(d) for d in results]
results: List[FMPFinancialRatiosData] = []
for item in data:
new_item = {to_snake_case(k).replace("ttm", ""): v for k, v in item.items()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-04-19 at 4 18 17 PM

Looks like we need to add "_" to the str.replace(), and "dividend_yiel_percentage" is a specialty for the "TTM" response. It's popped because it is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice yup done!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also have no idea why this one keeps failing on the test runs
image
lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice yup done!

"dividend_yield_percentage" can be dropped. This is redundant.

PE ratio and peg ratio, maybe some others are also being duplicated.

Screenshot 2024-04-19 at 10 00 58 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also have no idea why this one keeps failing on the test runs

I think this is because of way the requests are structured - the nested async requests within the callback.

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