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

Bug/2583 #2671

Merged
merged 14 commits into from
Sep 30, 2022
Merged

Bug/2583 #2671

merged 14 commits into from
Sep 30, 2022

Conversation

catalintoma
Copy link
Contributor

@catalintoma catalintoma commented Sep 29, 2022

Description

How has this been tested?

  • Added YF model tests
  • Added tests for the denomination helper

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.

@catalintoma
Copy link
Contributor Author

@DidierRLopes can you add the 'bug' label for this? If there is a way for me to set it on PR creation, please let me know to pay more attention.

I marked this as draft since I still want to improve the unit tests, but I am curious what you think about this approach. I've kind of wondered into to unknown with this with my limited Python skills, but felt there was room for refactoring all the places where we deal with converting denominations.

@colin99d colin99d marked this pull request as ready for review September 29, 2022 16:53
@colin99d colin99d marked this pull request as draft September 29, 2022 16:53
@colin99d colin99d added the bug Fix bug label Sep 29, 2022
@colin99d colin99d self-requested a review September 29, 2022 16:54
@colin99d
Copy link
Contributor

Looks great so far! Nice work.

@DidierRLopes
Copy link
Collaborator

This is looking very nice @catalintoma ! Keep up the good work! 🤝

@catalintoma catalintoma marked this pull request as ready for review September 30, 2022 10:19
@catalintoma
Copy link
Contributor Author

catalintoma commented Sep 30, 2022

@colin99d
I've requested a review, I haven't seen anything else to fix.
Main refactoring's are around:

  • Using helper for denomination transforms
  • Fix YF model not returning raw data (numbers) and also not using correct 'thousands' denomination
  • Fix YF view not exporting raw data

@colin99d
Copy link
Contributor

Perfect! Will do later today.

Copy link
Contributor

@colin99d colin99d left a comment

Choose a reason for hiding this comment

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

Great work man! Thanks for this pr.

@colin99d colin99d merged commit 07c08df into OpenBB-finance:main Sep 30, 2022
@catalintoma catalintoma deleted the bug/2583 branch September 30, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants