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

remove unicode characters from demo script filenames #5067

Closed

Conversation

giokara-oqton
Copy link

📚 Context

Please describe the project or issue background here

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change
    • Removes the Unicode characters from demo script filenames.

Revised:

Screenshot 2022-08-01 at 11 14 49

Current:

Screenshot 2022-08-01 at 11 15 08

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@giokara-oqton giokara-oqton requested a review from a team August 1, 2022 09:17
@sfc-gh-tszerszen
Copy link
Contributor

sfc-gh-tszerszen commented Aug 1, 2022

@giokara-oqton thanks for the PR 👍 I want others to tell they opinion on this, but I feel like this gives people more accessibility. Can you tell us how to reproduce the error coused by unicode characters?

@giokara-oqton oh, ok, I can see it's related to Bazel issue

@vdonato
Copy link
Collaborator

vdonato commented Aug 1, 2022

CC @jrieke as I believe this will come down to a product decision. If I'm remembering correctly, this issue is known, but work on fixing it wasn't prioritized as we believed the extent of the impact was relatively minor.

@jrieke
Copy link
Collaborator

jrieke commented Aug 1, 2022

Hey @giokara-oqton,

thanks for this PR! I think we want to keep the emojis in the streamlit hello demo though because the demo is an essential part of getting to know Streamlit and the emojis make it so much better. Nevertheless, we want to help you! What's your use case for using streamlit + bazel? Maybe that helps us find a workaround for you.

We do also want to find another way of adding emojis/icons to pages. But we first have to come up with a solid API to do this (and it's not our no. 1 priority right now), so it will probably take a few months.

@giokara-oqton
Copy link
Author

Hi @jrieke, not sure if it's clear from above, but this PR is not about removing emojis from the demo pages. The only change is removing the emojis from the filenames of the demo pages. Is this acceptable to you? Any changes on how emojis are supported within pages can be done separately I would expect.

Bazel is the chosen build system within our organisation and so any streamlit application needs to be run through Bazel. Currently we have issues using streamlit because of the emojis (non-ASCII characters) in the filenames.

@jrieke
Copy link
Collaborator

jrieke commented Aug 8, 2022

Oh sorry for the delay. Streamlit reads the emojis from these filenames to set them in the multipage app. So if we remove them from the filenames, it will also remove them from the sidebar navigation in the streamlit hello demo. There's currently no other way to set them. I.e. right no we can't merge this PR or remove the emojis from the filenames.

@jrieke jrieke closed this Aug 8, 2022
@AkaZn
Copy link

AkaZn commented Aug 8, 2022

Adding another +1 to this PR here. We are also currently stuck on streamlit 0.9 since bazel can't handle these file names at all. Is it possible to add the emoji as suggestion/comment inside the code instead? I know there are not that many bazel users but it does feel bad not able to use newer version due to example files :(.

@giokara-oqton
Copy link
Author

So the solution for those using Bazel and do not require newer functionality like emojis in the page, seems to be downgrading for now... For us, downgrading to 1.9.2 was sufficient (no emojis in the demo pages).

@getim
Copy link

getim commented Aug 9, 2022

This is blocking an upgrade to 1.11.1 for us as well. Considering that version patches a security vulnerability, we would really like to update, and being blocked by emojis in demo files indeed feels annoying.

@haugland-stripe
Copy link

We are in exactly the same position - can't upgrade to fix the security issue due to bazel's poor unicode support.

@jrieke jrieke reopened this Aug 10, 2022
@jrieke
Copy link
Collaborator

jrieke commented Aug 10, 2022

Hey all! We discussed this again since we got a lot of similar complaints. We decided that we'll merge this and remove the emojis from all filenames for now. Thanks to all here for surfacing your feedback! ❤️

@jrieke
Copy link
Collaborator

jrieke commented Aug 10, 2022

Ok turns out we can't merge this right away because of a merge conflict 😉 So I'm closing this and we'll replicate the same changes in a separate PR, so that we can merge quickly and get this into the upcoming 1.12.0 release on Thursday.

@jrieke jrieke closed this Aug 10, 2022
@giokara-oqton giokara-oqton deleted the close-issue-5066 branch August 10, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with Unicode characters in demo scripts
8 participants