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

Fixing exit command on forecast menu #3421

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

simmonsj330
Copy link
Contributor

Description

Fixes #3415.

The forecast menu exhibits a bug where if the user uses the exit command, they are directed back to the base menu. This occurs only when loading in data from another menu like stocks with the following commands stocks/load aapl/forecast. However, this error does not occur if they user navigates to the forecast menu first to load in their own dataset. Additionally, this error does not occur if the user inputs a ticker from another menu, navigates to the base menu, and then navigates to the forecast menu.

Source of the bug

This error occurs due to the number of times quit is called relative to the menu path. The exit command is used to leave the entire application and the backend of the exit command is simply X number of quits where X represents how "deep" in the terminal the user is, i.e what menu or sub-menu. For example, if the user is at the crypto/dd menu, then they are 3 deep: base->crypto->dd.
When applying this to forecasting where the user loads the ticker aapl and then calls forecasting, it is synonymous to being 3 menus deep because the user is going from base->stocks->forecasting. This seems to be true because if the user uses the quit command, they will be directed back to the stocks menu, and then if they use it twice more, they will be directed back to the base menu and then out of the terminal.
However, the exit command directs the user back only to the base menu. This means that there is one missing quit call. This occurs because forecasting is a primary menu option off of the base menu and its path is forecast/, (base->forecast). The exit function utilizes this path and thus only calls quit twice.

Solution

The solution is to simply increase the total number of quits used when exiting. This will cover the forecast edge case when there are not enough quits. However, this means that all other exit calls will have one additional, unneeded quit. This does not seem to cause any errors or lead to adverse functionality.

Screen Shot 2022-11-14 at 7 42 16 PM

Others

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

@simmonsj330 simmonsj330 added the bug Fix bug label Nov 15, 2022
@simmonsj330 simmonsj330 self-assigned this Nov 15, 2022
@reviewpad reviewpad bot added the feat XS Extra small feature label Nov 15, 2022
Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

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

works for me both from /forecast and /stocks/forecast

Copy link
Contributor

@martinb-ai martinb-ai left a comment

Choose a reason for hiding this comment

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

works for me too

Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

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

So actually thinking about this more -- this is the only place this is needed. And it arises because we call PATH = "/forecast/" in this controller.

Instead of rewriting the base class, which changes every class (hence why you are failing 100 tests), I think we can overwrite the exit function for forecasting menu only.

@martinb-ai martinb-ai merged commit 8e18d47 into OpenBB-finance:main Nov 16, 2022
@simmonsj330 simmonsj330 mentioned this pull request Nov 27, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug feat XS Extra small feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]Exit does not exit from the terminal while in forecast menu
3 participants