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

Fix economy bugs #3144

Merged
merged 36 commits into from
Nov 7, 2022
Merged

Fix economy bugs #3144

merged 36 commits into from
Nov 7, 2022

Conversation

hjoaquim
Copy link
Contributor

@hjoaquim hjoaquim commented Oct 28, 2022

Description

  • Summary of the change / bug fix // Link # issue, if applicable. // Screenshot of the feature or the bug before/after fix, if applicable.
  1. Fix [Bug] /economy/ $ ycrv --source investpy #3082 --> autocompletion and sources were not congruent. Removed the Investing/investpy source until it's not fixed.
    image

  2. Fix [Bug] /economy/ $ events --country united kingdom #3083 --> autocompletion to use "_" in countries with spaces. Removed the Investing/investpy source until it's not fixed.
    image

  3. Fix [Bug] /economy/ $ edebt --limit 10 #3084 --> added rank column based on debt.
    image

  4. Fix [Bug] /economy/ $ treasury --show parameters #3085 --> autocompletion was wrongly showing options for treasure --show (it is a boolean flag)
    image

  5. Fix [Bug] /economy/ $ plot --y1 T20YIEM DFII10 #3087 --> autocompletion was using spaces instead of comma.
    image

  6. Fix [Bug] /economy/ $ qa #3150 --> fixed verification on empty datasets
    image

  7. Fix [Bug] /economy/ $ eval spread = dgs2 - dgs5 #3120 --> fixed the docstring and website docs
    image

  8. Fix [Bug] economy/spread doesn't exist #3148 --> commented out spread so it does not appear on autocompletion (not the best thing to do but followed what was done for this command - probably someone already trying to have an alternative data source to investpy)
    image

  • Relevant motivation and context.
    🐛 💀

  • List any dependencies that are required for this change.

    • NA

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.
  • Make sure affected commands still run in terminal
  • Ensure the SDK still works
    • Changes on controllers shouldn't impact SDK.
    • commodity_model is not used in the SDK.
    • from openbb_terminal.sdk import openbb still runs. Also, tried some random commands and is all good.
  • Check any related reports
    • Changes on controllers shouldn't impact SDK.

Checklist:

Others

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

@hjoaquim hjoaquim added the bug Fix bug label Oct 28, 2022
@hjoaquim hjoaquim self-assigned this Oct 28, 2022
@hjoaquim hjoaquim marked this pull request as ready for review October 31, 2022 16:22
@jmaslek
Copy link
Collaborator

jmaslek commented Nov 1, 2022

Hey great job here! I have a couple small things going through.

  • When source investpy is selected, can we print a newline in between :D
  • I am getting some funky fred behavior where when I do the commands you linked, the FRED chart pops up, but doing plot --y1 T20YIEM shows nothing (and also prints quite a few terminal newlines):

Screen Shot 2022-10-31 at 8 57 51 PM

  • small extra point, but it looks like there is the ENABLE_EXIT_AUTO_HELP is only on some functions. We should have all or none.

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Nov 1, 2022

Hey great job here! I have a couple small things going through.

* When source investpy is selected, can we print a newline in between :D

* I am getting some funky fred behavior where when I do the commands you linked, the FRED chart pops up, but doing plot --y1 T20YIEM shows nothing (and also prints quite a few terminal newlines):
Screen Shot 2022-10-31 at 8 57 51 PM
* small extra point, but it looks like there is the ENABLE_EXIT_AUTO_HELP is only on some functions.  We should have all or none.

Hey @jmaslek, thanks for reviewing this!

  • The investpy thing, like this??:

image

  • Regarding the FRED chart, could you give me the sequence of commands used pls? Tried a bunch of different commands and then the plot seems to work just fine:

image

On the lines printed after using the plot I think that's the right behavior:

1 empty line for user input and start of the command output.
1 empty line for output (that does not exist, just the plot)
1 empty line between command output and the user input

I think this is valid across the terminal - does this make sense?

  • ENABLE_EXIT_AUTO_HELP not sure if I agree on this (at first I was also feeling this behavior was looking a bit odd) but I guess the intention behind this was to show the user the updated Stored datasets, that's why the menu is re-printed when you use one of this: macro, treasury, fred or index. What do you think?

@hjoaquim hjoaquim requested a review from jmaslek November 1, 2022 10:33
@jmaslek
Copy link
Collaborator

jmaslek commented Nov 1, 2022

  • The investpy thing, like this??:

image

Nah. In your screenshot, when you ran

events --source investing
events --source investing

Should add be a newline between the two.

  • Regarding the FRED chart, could you give me the sequence of commands used pls? Tried a bunch of different commands and then the plot seems to work just fine:
fred T20YIEM,DFII10
plot --y1 T20YIEM

Still not updating for me

  • ENABLE_EXIT_AUTO_HELP not sure if I agree on this (at first I was also feeling this behavior was looking a bit odd) but I guess the intention behind this was to show the user the updated Stored datasets, that's why the menu is re-printed when you use one of this: macro, treasury, fred or index. What do you think?

I see. That kinda makes sense. I would prefer just showing the loaded datasets, but that is our of scope here.

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Nov 7, 2022

@jmaslek I've reverted this commit. If you agree, I'll keep working on this on a different PR; this one is too big already.

@hjoaquim hjoaquim merged commit 46afc9e into OpenBB-finance:main Nov 7, 2022
@hjoaquim hjoaquim deleted the fix_economy_bugs branch November 16, 2022 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment