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
Normalize main script path in st.switch_page
and st.page_link
#8103
Conversation
st.switch_page
and st.page_link
@@ -139,6 +139,7 @@ def switch_page(page: str) -> NoReturn: # type: ignore[misc] | |||
raise NoSessionContext() | |||
|
|||
main_script_path = os.path.join(os.getcwd(), ctx.main_script_path) | |||
main_script_path = os.path.normpath(main_script_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just throwing a couple thoughts to consider before merging.
- The problem is the script path is not our desired state, shouldn't we just do
os.path.normpath(ctx.main_script_path)
instead of normpath the whole thing? I guess it's the same outcome, but perhaps we are specifically identifying the problem. - Should we actually be normalizing this at the very beginning on load, so that we can assume the main script path is set correctly (and avoid this issue again).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I also thinking about this (normalizing main_script_path
in context, and not on individual elements level, although here tried to keep changes as small as possible for the fix to not break something else unintentionally).
Absolutely agree it is worth doing!
I am happy to volunteer for that, just want to do it after this merge more carefully to potential side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think I would do (1) narrow this more to the specific issue (the fact that the ctx.main_script_path
needs to be normpath).
We can then consider expanding to my question for (2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Might be good to check this on a windows computer. I don't have one around right now, but could potentially do it later at home.
Describe your changes
Normalize the main path before using it in
st.page_link
andst.switch_page
.Close: #8070
GitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.