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

Fixes bug #1907 scraping management data #1914

Merged
merged 6 commits into from
Jun 8, 2022

Conversation

simmonsj330
Copy link
Contributor

Description

Fixes management bug #1907 where it was not scraping the management data when calling 'mgmt' for any ticker.

  • [ x] Summary of the change / bug fix.
  • [ x] Link # issue, if applicable.
  • Screenshot of the feature or the bug before/after fix, if applicable.
  • [ x] Relevant motivation and context.
  • List any dependencies that are required for this change.

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Checklist:

Others

  • [ x] I have performed a self-review of my own code.
  • [ x] I have commented my code, particularly in hard-to-understand areas.
  • [ x] My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • [ x] New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@simmonsj330 simmonsj330 added the bug Fix bug label Jun 7, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Jun 7, 2022

Welcome aboard!

Tests can be rerun using pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected. (same for the view test)

Only other comment would be that in the case when data is not found, there is a print in both model and view, resulting in two prints

2022 Jun 07, 16:01 (🦋) /stocks/fa/ $ mgmt
No management information in Business Insider for IONQ

Data not available

Could limit to one

@Chavithra
Copy link
Contributor

Chavithra commented Jun 8, 2022

Hi @simmonsj330,

Here I remind the points from @jmaslek and add an extra.

FIXING PYTEST
From @jmaslek.
Running the following command to get rid of the pytest error :
pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

REMOVING ERROR MESSAGE
From @jmaslek.
We should remove the error message from the model.
Maybe it should raise an exception that can be caught by the view.

FUNCTION REFACTORING
The original function is way too long if you have some extra time it worth refactoring.
Rule of thumb that I use : if it takes more than 30 seconds to understand a function it is probably too complex.
We can split the function to make it simpler to understand.

BUG
The bug itself seems to be fixed.

@Chavithra Chavithra added the do not merge Label to prevent pull request merge label Jun 8, 2022
@jmaslek jmaslek removed the do not merge Label to prevent pull request merge label Jun 8, 2022
@simmonsj330 simmonsj330 merged commit 7527e91 into OpenBB-finance:main Jun 8, 2022
@simmonsj330 simmonsj330 deleted the management_bug_fix branch June 14, 2022 05:30
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