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

105 move settings to session state #111

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

markushal
Copy link
Collaborator

@markushal markushal commented Nov 15, 2023

I moved the settings dictionary to st.session_state["settings"]. keep in mind that unittests do not work for functions that call st.session_state. This is why I removed the legacy calculate_results function and the associated test.

We should try to set up the streamlit testing framework #112

@joAschauer I hope these changes do not conflict with the work you are currently doing

@markushal markushal self-assigned this Nov 15, 2023
@markushal markushal marked this pull request as ready for review November 15, 2023 11:38
@markushal markushal requested a review from joAschauer November 15, 2023 11:38
@@ -147,8 +109,10 @@ def aggregate_costs(res_details: pd.DataFrame) -> pd.DataFrame:

# Settings:
def create_sidebar(api: PtxboaAPI):
if "settings" not in st.session_state:
st.session_state["settings"] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason you put settings in a dictionary inside session state and do not place each entry in settings in the session state as an individual key? This would make code that accesses a single setting value more concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm. I thought it would be nice to have all settings grouped together in a dictionary. But you are right, there is no real need for this. Good point.

@markushal markushal requested a review from joAschauer November 15, 2023 15:02
@joAschauer joAschauer merged commit 20c0af9 into develop Nov 15, 2023
3 checks passed
@joAschauer joAschauer deleted the 105-move-settings-to-session_state branch November 15, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants