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 settings not updating right away #2851

Merged
merged 10 commits into from
Oct 18, 2022
Merged

Fix settings not updating right away #2851

merged 10 commits into from
Oct 18, 2022

Conversation

colin99d
Copy link
Contributor

@colin99d colin99d commented Oct 14, 2022

Description

Fixes #2676

Currently the TZ setting does not update until the user restarts the terminal. Every other setting updates instantly. I fixed this by updating the environment variable when a user runs this command. Additionally, a bad environment key can currently brick the terminal. I fixed this for some places, but a lot of fixes could still be used.

How has this been tested?

This affects code running at start so it could cause a lot of issues. However, it is only additional error handling and setting an environment variable, which minimizes risk,

  • Make sure affected commands still run in terminal
    • To ensure the tz commands update instantly I ran python terminal.py settings tz [TIMEZONE] / h and confirm the new tz listed matches the one you used.
  • Ensure the api still works
    • Confirmed the API can still start normally to ensure the error handling around environment variable loading did not affect the terminal. To confirm the charts still appear normally I ran:
      • from openbb_terminal.api import openbb
      • openbb.stocks.candle("TSLA", chart=True)
  • Check any related reports

Checklist:

Others

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

@colin99d colin99d added the bug Fix bug label Oct 14, 2022
@colin99d colin99d requested a review from Chavithra October 14, 2022 18:42
@colin99d colin99d added the do not merge Label to prevent pull request merge label Oct 14, 2022
@colin99d colin99d marked this pull request as draft October 14, 2022 18:54
@jmaslek
Copy link
Collaborator

jmaslek commented Oct 14, 2022

I think this works, but couldn't you just do set_key(.., export=True)

@colin99d
Copy link
Contributor Author

Yeah I did this wrong. Turns out timezone is the only broken one. My local branch has a lot more progress in fixing.

@colin99d colin99d removed the do not merge Label to prevent pull request merge label Oct 17, 2022
@colin99d
Copy link
Contributor Author

I think this works, but couldn't you just do set_key(.., export=True)

When I tried this time zone did not update right away.

@colin99d colin99d marked this pull request as ready for review October 17, 2022 05:45
@jmaslek jmaslek merged commit 84b0435 into main Oct 18, 2022
@piiq piiq deleted the fix_late_upadte branch November 9, 2022 18:34
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.

[Bug] Terminal settings are not being read from the file they are written
2 participants