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

Trading hours stock feature #1697

Merged
merged 26 commits into from
May 11, 2022
Merged

Trading hours stock feature #1697

merged 26 commits into from
May 11, 2022

Conversation

buahaha
Copy link
Contributor

@buahaha buahaha commented Apr 14, 2022

Description

  • Summary of the change / bug fix.

    • Add checking for available markets that are open, closed, list all and check one. Also you can load one security to know if it's currently been traded.
  • Link # issue, if applicable.

  • Screenshot of the feature or the bug before/after fix, if applicable.

    • Add new context box (menu) to look for securities. Might work for all of OpenBB tickers if implemented in full.
      Screenshot 2022-04-14 at 04 31 26
  • Relevant motivation and context.

    • You should. know when you can trade, and when you can't.
  • List any dependencies that are required for this change.

    • None

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
    • Manual checking of few possible use cases...
  • Provide instructions so we can reproduce.
    • /stocks/th/open
    • /stocks/th/closed
    • /stocks/th/all
    • /stockc/th/symbol MSFT
  • 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/....

@Chavithra Chavithra self-requested a review April 14, 2022 10:34
@jmaslek jmaslek added the feat S Small T-Shirt size Feature label Apr 14, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Apr 14, 2022

Nice one.

Couple comments. First is that the tradinghours still lives on the main menu. I think this should just be shown under stocks

Screen Shot 2022-04-14 at 9 15 07 AM

And in the stocks menu, I think we can put this up the top section (see image below). (thoughts @DidierRLopes )

Screen Shot 2022-04-14 at 9 17 41 AM

When I load in an exchange, we should do some capitalization on the input, so that I could enter nyse or NYSE and it would understand

2022 Apr 14, 09:20 (🦋) /tradinghours/ $ exchange nyse

No exchange data loaded.
Make sure you picked proper exchange symbol.

2022 Apr 14, 09:20 (🦋) /tradinghours/ $ exchange NYSE
*works*

Also for the exchange function, is it possible to auto complete that field like we have throughout the terminal?

And using -h flag throws a small error:

2022 Apr 14, 09:20 (🦋) /tradinghours/ $ exchange -h
usage: exchange [-n EXCHANGE] [-h]

Select the exchange you want see open hours for

options:
  -n EXCHANGE, --name EXCHANGE
                        Exchange short name (default: None)
  -h, --help            show this help message (default: False)

Make sure 'symbol' is selected.

@jmaslek
Copy link
Collaborator

jmaslek commented Apr 15, 2022

Thanks for making some changes! I am still running into an issue with exchange

2022 Apr 15, 11:34 (🦋) /th/ $ exchange NYSE
Error: 'NYSE'

Error: 'list' object has no attribute 'empty'

Additionally, exit needs one more quit, as it just takes us back to the root. (Also on your model and view, check line 1, one of them still says forex)

Copy link
Contributor

@Chavithra Chavithra left a comment

Choose a reason for hiding this comment

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

Thanks for contributing !

Looks good overall.

I wonder why this file was deleted ?
website/content/terminal/stocks/_index.md

@@ -1,5 +0,0 @@
---
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 reason to remove this file ?

@@ -0,0 +1,1093 @@
from datetime import time
Copy link
Contributor

@Chavithra Chavithra Apr 19, 2022

Choose a reason for hiding this comment

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

I would replace this python file by a yaml (or .ini or json...) file and load it with a function.

Since it's a data file and not code.

Plus this will allow not having to ignore this file from the codespell check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@Chavithra Chavithra left a comment

Choose a reason for hiding this comment

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

I think the only issue left is the linter complaining about some spagetti code here :

  • openbb_terminal/stocks/tradinghours/bursa_model.py::check_if_open

https://github.com/OpenBB-finance/OpenBBTerminal/runs/6080772850?check_suite_focus=true

@Chavithra Chavithra self-requested a review May 11, 2022 11:44
Copy link
Contributor

@Chavithra Chavithra left a comment

Choose a reason for hiding this comment

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

Looks ready for merging.

Copy link
Collaborator

@DidierRLopes DidierRLopes left a comment

Choose a reason for hiding this comment

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

Not yet. Before we merge can we have:

  1. AAPL shouldn't be loaded by default, should be empty.
  2. Autocomplete with 10k tickers doesn't work well, it's too slow and not worthy - we had this discussion internally already.3.
  3. The colouring scheme is wrong parameters and the value should be [param] and normal by default.4.
  4. the menu text is not properly aligned and there are 2 white lines instead of 1.5.
  5. When parsing an argument let's use .upper() so we can accept a ticker even with lowercase

Screenshot 2022-05-11 at 12 55 53

@Chavithra
Copy link
Contributor

Chavithra commented May 11, 2022

Not yet. Before we merge can we have:

  1. AAPL shouldn't be loaded by default, should be empty.
  2. Autocomplete with 10k tickers doesn't work well, it's too slow and not worthy - we had this discussion internally already.3.
  3. The colouring scheme is wrong parameters and the value should be [param] and normal by default.4.
  4. the menu text is not properly aligned and there are 2 white lines instead of 1.5.
  5. When parsing an argument let's use .upper() so we can accept a ticker even with lowercase
Screenshot 2022-05-11 at 12 55 53

Changed were done.
I think it's ready now.

Copy link
Collaborator

@DidierRLopes DidierRLopes left a comment

Choose a reason for hiding this comment

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

Thanks Chavi!

@DidierRLopes DidierRLopes merged commit 33a041e into OpenBB-finance:main May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat S Small T-Shirt size Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants