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.

Sign up for GitHub

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

Jump to bottom

Update stocks_helper.py #2025

Merged
merged 10 commits into from
Jul 1, 2022
Merged

Update stocks_helper.py #2025

merged 10 commits into from
Jul 1, 2022

Conversation

pauljsymonds
Copy link
Contributor

When a stock is returned:

Market: CLOSEDSENIOR PLC 10p

Fixed to show:

Market: CLOSED
Company: SENIOR PLC 10p

Description

  • Summary of the change / bug fix.
  • Link # issue, if applicable.
  • Screenshot of the feature or the bug before/after fix, if applicable.
  • 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

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

@pauljsymonds
Update stocks_helper.py
485bbb2 
When a stock is returned:

Market: CLOSEDSENIOR PLC 10p

Fixed to show:

Market: CLOSED
Company: SENIOT PLC 10p
@pauljsymonds
Update stocks_helper.py
96ce402 
Added logic for US if source requires .US. without this logic Datetime is printed twice.
@pauljsymonds
Update stocks_helper.py
fe44c4c 
Updated logic to fix pyLint Error.
@JerBouma
Copy link
Contributor

JerBouma commented Jul 1, 2022

Why does this bug occur and what does that company mean exactly?

@pauljsymonds
Update stocks_helper.py
6e88c09
@pauljsymonds
Update stocks_helper.py
a2a8796 
Further testing revealed Upper and Lower case issues, if .upper() is used it throws errors.

This function uses yfinance to return info data for tickers. 

Yahoo does not have .US suffix for US stocks but other feeds may (will) have this.

Some testing gave the following:

Stock:                                                                                                                                                            │
                                                                                                                                                              
Datetime:                                                                                                                                                         
Timezone:                                                                                                                                                         
Exchange:                                                                                                                                                         
Market:                                                                                                                                                           
Currency:                                                                                                                                                         
Company:  

or:

Datetime: 2022 Jul 01 07:51
Timezone: Europe/London
Exchange: LSE
Currency: GBp
Market:   OPENSENIOR PLC 10P


or this:

Datetime: 
Datetime: 2022 Jul 01 02:44
Timezone: America/New_York
Currency: USD
Market:   CLOSED
Company:  

Final Result is this:

Datetime: 2022 Jul 01 02:51
Timezone: America/New_York
Currency: USD
Market:   CLOSED
Company:  Apple Inc.


I don't think this function is overly complex and could do with a refactor at some point.
@pauljsymonds
Copy link
Contributor Author

Why does this bug occur and what does that company mean exactly?

This is just ticker info returned from yfinance and displayed when stock is loaded.

The formatting was incorrect and in some instances it displayed extra info.

I think it still requires some work but the initial bugs identified are fixed.

@JerBouma
Copy link
Contributor

JerBouma commented Jul 1, 2022

Alright, makes sense to do it this way then.

@JerBouma JerBouma self-requested a review July 1, 2022 07:26
@DidierRLopes DidierRLopes added the bug Fix bug label Jul 1, 2022
@pauljsymonds
Update stocks_helper.py
cf3e557 
fixed Black error
@pauljsymonds
Copy link
Contributor Author

RE the Pylint error, I am 99% sure I tested this with both lower and uppercase in the terminal and I got an issue hence the change to the logic:

************* Module openbb_terminal.stocks.stocks_helper
13
openbb_terminal/stocks/stocks_helper.py 1006: R1727 Boolean condition ''.US' or '.us' in ticker' will always evaluate to ''.US'' (condition-evals-to-constant)
14

I also tried a .upper() but this function did not like a str.

@colin99d colin99d self-requested a review July 1, 2022 12:31
colin99d
colin99d approved these changes Jul 1, 2022
View reviewed changes
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.

Looks great! Will merge once tests pass. Thanks for this!!

@colin99d colin99d merged commit 0f0e769 into OpenBB-finance:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin99d colin99d colin99d approved these changes

@JerBouma JerBouma Awaiting requested review from JerBouma

Assignees
No one assigned
Labels
bug Fix bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pauljsymonds @JerBouma @colin99d @DidierRLopes