-
Notifications
You must be signed in to change notification settings - Fork 4k
Add pathlib support #9711
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
Merged
Merged
Add pathlib support #9711
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukasmasuch
reviewed
Oct 24, 2024
jrieke
added a commit
that referenced
this pull request
Nov 2, 2024
## Describe your changes `lib/streamlit/elements/image.py` contained a bunch of utils functions that were imported in other files, which caused cyclic imports in #9711. This PR moves all of these functions into `lib/streamlit/elements/lib/image_utils.py` and makes all other files import directly from there. Unfortunately, this PR didn't remove all cyclic imports. But it's still worth merging this to clean up our codebase a bit more. ## GitHub Issue Link (if applicable) Just a refactor, no new functionality or tests added. ## 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. --------- Co-authored-by: Johannes Rieke <[email protected]>
lukasmasuch
reviewed
Nov 2, 2024
lukasmasuch
approved these changes
Nov 2, 2024
Collaborator
lukasmasuch
left a comment
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 👍
Contributor
|
Have you considered using |
jrieke
added a commit
that referenced
this pull request
Nov 5, 2024
## Describe your changes This adds pathlib support for `AppTest.from_file` and `st.components.v1.declare_component`. Follow-up for #9711. ## GitHub Issue Link (if applicable) ## Testing Plan Added Python unit tests for both functions. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Johannes Rieke <[email protected]>
edegp
pushed a commit
to edegp/streamlit
that referenced
this pull request
Jan 19, 2025
## Describe your changes `lib/streamlit/elements/image.py` contained a bunch of utils functions that were imported in other files, which caused cyclic imports in streamlit#9711. This PR moves all of these functions into `lib/streamlit/elements/lib/image_utils.py` and makes all other files import directly from there. Unfortunately, this PR didn't remove all cyclic imports. But it's still worth merging this to clean up our codebase a bit more. ## GitHub Issue Link (if applicable) Just a refactor, no new functionality or tests added. ## 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. --------- Co-authored-by: Johannes Rieke <[email protected]>
edegp
pushed a commit
to edegp/streamlit
that referenced
this pull request
Jan 19, 2025
## Describe your changes Adds support for pathlib.Path to the following commands: - st.image - st.audio - st.video - st.set_page_config - st.switch_page - st.page_link - st.logo - st.html - st.Page - st.chat_message I also refactored some code snippets that use test assets to use pathlib. ## GitHub Issue Link (if applicable) ## Testing Plan I added small unit tests and e2e tests for most commands. Some notes: - st.audio didn't have e2e tests for local files at all, so I added some. - st.switch_page doesn't seem to have unit tests, so I only added an e2e test. - st.page_link already had a unit test for pathlib. - For st.logo, I didn't add an e2e test because you can only test it once per app, and adding another app just for this seemed overkill. It uses the same logic as st.image under the hood though. - st.html already had a unit test for pathlib. It didn't have an e2e test for files at all, so I added some (both for str paths and for pathlib). --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Johannes Rieke <[email protected]>
edegp
pushed a commit
to edegp/streamlit
that referenced
this pull request
Jan 19, 2025
## Describe your changes This adds pathlib support for `AppTest.from_file` and `st.components.v1.declare_component`. Follow-up for streamlit#9711. ## GitHub Issue Link (if applicable) ## Testing Plan Added Python unit tests for both functions. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Johannes Rieke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
change:feature
PR contains new feature or enhancement implementation
impact:users
PR changes affect end users
security-assessment-completed
Security assessment has been completed for PR
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Adds support for pathlib.Path to the following commands:
I also refactored some code snippets that use test assets to use pathlib.
GitHub Issue Link (if applicable)
Testing Plan
I added small unit tests and e2e tests for most commands. Some notes:
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.