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

Improve OpenBB API for investment research reports #1753

Merged
merged 23 commits into from
May 3, 2022
Merged

Improve OpenBB API for investment research reports #1753

merged 23 commits into from
May 3, 2022

Conversation

DidierRLopes
Copy link
Collaborator

@DidierRLopes DidierRLopes commented Apr 30, 2022

Improved a LOT of small things on the API:

  • Some typos
  • Some bad imports due to the logging decorator which broke implemented logic to get models funcs from notebook
  • Create a very nice and slick equity report (can be improved on, it's still a POOC)

@piiq @jose-donato this is silly but how can we have the OpenBB_reports_logo.png image appearing on the report when this is run from the reports controller?

@DidierRLopes DidierRLopes added the bug Fix bug label Apr 30, 2022
@DidierRLopes DidierRLopes requested a review from piiq April 30, 2022 21:58
@DidierRLopes DidierRLopes changed the title Improve Dark Pool API for Reports Improve OpenBB API for investment research reports May 1, 2022
@jose-donato
Copy link
Contributor

The image should be good now. Converted to base64 and then rendered the image.

also fixed some other HTML stuff

Copy link
Contributor

@piiq piiq left a comment

Choose a reason for hiding this comment

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

We'll need to get rid of the .ipynb_checkpoints that accidentally slipped into the PR

I've left a couple of comments additionally, but nothing critical

@@ -47,11 +48,9 @@
)

# Models
# NOTE: The raw function is used here to point to the commons path where all the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember why I've implemented it like this 😂

@@ -28,7 +28,7 @@ class ReportController(BaseController):
reports_folder = os.path.dirname(os.path.abspath(__file__))

report_names = [
notebooks.strip(".ipynb")
notebooks[:-6]
Copy link
Contributor

Choose a reason for hiding this comment

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

not critical to me, but why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took me some time to figure out that the strip was problematic. I had misunderstood it's usage. See
Screenshot 2022-05-03 at 11 10 27

Copy link
Contributor

Choose a reason for hiding this comment

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

argh

@@ -207,7 +186,7 @@ def darkpool_otc(
num: int,
promising: int,
tier: str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to have a default kwarg for the tier? It's a string and giving a default kwarg would make it easier to understand what it looks like

@DidierRLopes DidierRLopes requested a review from piiq May 3, 2022 10:26
@DidierRLopes
Copy link
Collaborator Author

@piiq should be good now :)

@piiq
Copy link
Contributor

piiq commented May 3, 2022

@DidierRLopes no, it's not
image

@DidierRLopes
Copy link
Collaborator Author

Should be now @piiq

@piiq piiq merged commit b7f8fbe into main May 3, 2022
@piiq piiq deleted the darkpoolapi branch May 3, 2022 13:44
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