Skip to content

Conversation

@jrieke
Copy link
Collaborator

@jrieke jrieke commented Oct 22, 2024

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.

@sfc-gh-jrieke sfc-gh-jrieke added security-assessment-completed Security assessment has been completed for PR impact:users PR changes affect end users change:feature PR contains new feature or enhancement implementation labels Oct 22, 2024
@jrieke jrieke marked this pull request as ready for review October 23, 2024 16:25
@jrieke jrieke changed the title [WIP] Add pathlib support Add pathlib support Oct 23, 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]>
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jrieke jrieke merged commit e3b751d into develop Nov 2, 2024
@jrieke jrieke deleted the feature/pathlib-support branch November 2, 2024 09:20
@Dev-iL
Copy link
Contributor

Dev-iL commented Nov 3, 2024

Have you considered using Path's .as_posix() method instead of str()? This standardizes the outputs of the operation across OSs.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants