Keep support for ICO formats by defaulting to PNG if necessary #4343
Conversation
This file contains 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
kmcgrady
reviewed
Feb 2, 2022
| @@ -207,6 +207,8 @@ def _normalize_to_bytes(data, width, output_format): | |||
| if output_format.lower() == "auto": | |||
| ext = imghdr.what(None, data) | |||
| mimetype = mimetypes.guess_type("image.%s" % ext)[0] | |||
| if mimetype is None: | |||
There was a problem hiding this comment.
I realize I wrote this, but I would just add a comment that suggests that there's no other options, just try to convert it (would require Pillow support).
| import os, fnmatch | ||
|
|
||
|
|
||
| def is_assets_favicon_path(path): |
There was a problem hiding this comment.
Haha this code got complicated, I would just add a comment on the reason why this is happening (we should note that this later can be a e2e_test_util library
| it("sets the page favicon with ico file", () => { | ||
| cy.get("link[rel='shortcut icon']") | ||
| .should("have.attr", "href") | ||
| .should("contain", "png"); |
There was a problem hiding this comment.
This would produce a false negative. The default is a PNG if not specified, so if assets/favicon.ico is not found. it will default to a png 馃ぃ
kmcgrady
approved these changes
Feb 3, 2022
tconkling
added a commit
to tconkling/streamlit
that referenced
this pull request
Feb 7, 2022
# By Vincent Donato (3) and others # Via GitHub * develop: Add temporary fix to download tensorflow less than 2.8 but >=2.6 in order to fix test suite (streamlit#4372) Cloud plan titles updated in README (streamlit#4369) Actually save the query_string after a script run (streamlit#4334) Update Streamlit markdown to be more flexible with links (streamlit#4364) Keep support for ICO formats by defaulting to PNG if necessary (streamlit#4343) Remove some remaining usage of the term "report" (streamlit#4293) Improve error when streamlit is run via `python -m streamlit.cli` (streamlit#4359) # Conflicts: # lib/streamlit/app_session.py # lib/streamlit/script_runner.py
tconkling
added a commit
to tconkling/streamlit
that referenced
this pull request
Feb 7, 2022
* develop: ScriptRunner + AppSession type annotations (streamlit#4376) Remove config assertions and introduce warnings. (streamlit#4354) Relax assertions around streamlit program name (streamlit#4374) Add temporary fix to download tensorflow less than 2.8 but >=2.6 in order to fix test suite (streamlit#4372) Cloud plan titles updated in README (streamlit#4369) Actually save the query_string after a script run (streamlit#4334) Update Streamlit markdown to be more flexible with links (streamlit#4364) Keep support for ICO formats by defaulting to PNG if necessary (streamlit#4343) Remove some remaining usage of the term "report" (streamlit#4293) Improve error when streamlit is run via `python -m streamlit.cli` (streamlit#4359)
kmcgrady
pushed a commit
that referenced
this pull request
Feb 8, 2022
* Keep support for ICO formats by defaulting to PNG if necessary
vdonato
added a commit
that referenced
this pull request
Feb 15, 2022
* Up version to 1.5.0 * Update `react-json-view` and `vega` for dep security (#4352) `node-fetch` 2.6.1 has a severe vulnerability, these packages were using that version but had more recent versions using 2.6.7 which fixes the vulnerability. * Improve error when streamlit is run via `python -m streamlit.cli` (#4359) * Remove config assertions and introduce warnings. (#4354) * Add logger while removing assertion and changing corresponding test * Keep support for ICO formats by defaulting to PNG if necessary (#4343) * Keep support for ICO formats by defaulting to PNG if necessary * Update Streamlit markdown to be more flexible with links (#4364) * Adding in functionality to pass in props for class and target and other things * Adding in test * Relax assertions around streamlit program name (#4374) * Up version to 1.5.1 Co-authored-by: Amanda Walker <amanda@amandawalker.io> Co-authored-by: Vincent Donato <vincent@streamlit.io> Co-authored-by: willhuang1997 <whuang@streamlit.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
馃摎 Context
Please describe the project or issue background here
What kind of change does this PR introduce?
馃 Description of Changes
imghdrcannot determine the image, aim for the default mimetype as defined by "auto".No screenshot needed
馃И Testing Done
馃寪 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.
Credit goes to Ken as I just simply edited the e2e tests to be more based on the discussion of this link( https://gitter.im/cypress-io/cypress?at=5d49916c757b7b19c8666cbb)
where someone suggests: "Just in case someone is interested in the future:
cy.visit('/index.html').document().its('head').find('link[rel="icon"]').should('have.attr', 'href').should('eq', ;"