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

Don't warn about .png format favicons #3715

Closed
wants to merge 1 commit into from

Conversation

njsmith
Copy link
Contributor

@njsmith njsmith commented May 7, 2017

It looks like every browser can now handle .png favicons:

https://caniuse.com/#feat=link-icon-png

(When looking at that page, note that iOS Safari and Opera Mini don't
support .ico favicons either.) So I don't think sphinx needs to warn
about this anymore...

This is especially annoying because I build with -nW, which is great
for catching lots of real errors but means that currently it's
actually impossible for me to use a .png favicon, even though it's
smaller, easier to work with, and supported by everything except
sphinx.

@njsmith
Copy link
Contributor Author

njsmith commented May 7, 2017

Travis failed, but I don't see how the errors could be my fault... is this expected?

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Could you rebase this into stable branch? Then I will release this as 1.5.6 (or 1.6).

The errors on CI will be cleared if you rebased. (it comes from #3716)

if favicon and os.path.splitext(favicon)[1] != '.ico':
logger.warning('html_favicon is not an .ico file')
if favicon and os.path.splitext(favicon)[1] not in ['.ico', '.png']:
logger.warning('html_favicon is not an .ico or .png file')
Copy link
Member

Choose a reason for hiding this comment

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

Now many image formats are allowed to favicon, so there are no meaning to warn about file types here.
https://en.wikipedia.org/wiki/Favicon#Browser_implementation

How about removing this block?

@tk0miya tk0miya added this to the 1.5.6 milestone May 7, 2017
Support for at least .png and .gif is now essentially universal, and
other formats (.jpg, .svg, etc.) are relatively widespread.

This was only a warning, but it's common to run sphinx with warnings
turned into errors, so it caused problems.
@njsmith
Copy link
Contributor Author

njsmith commented May 7, 2017

It looks like the only way to change the target from master to stable is to close this pull request and recreate it.

@njsmith njsmith closed this May 7, 2017
@njsmith njsmith deleted the favicon-png-is-ok branch October 14, 2020 10:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants