Default favicon

[T4rk1n]

Adds a default favicon if no favicon is found in the assets folder.

To override the default favicon, you just need to put a favicon.ico file in the assets folder.

Resolves #152.

[ned2]

Added some comments, but no real issues. Good to see this getting attended to!

💃

[ned2]

Could this route be {}_favicon.ico? I feel like this would make it clearer that the route is serving a .ico file directly

[T4rk1n]

Yes probably clearer thanks.

[ned2]

I found this logic with the nested formats and conditional expression a little hard to parse initially. I'd have a preference for something more verbose but easier to read, like:

if self._favicon:
    favicon_url = self.get_asset_url(self._favicon)
else:
    favicon_url = '{}_favicon'.format(self.config.requests_pathname_prefix)
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(favicon_url)

[rmarren1]

This looks good. I found it a bit difficult to change the favicon.ico since it is (at least for me in Chrome 69) not cleared when the cache is deleted. It seems to take hours to days for this to change in some browsers according to some posts on SO.

Do you think we can implement a cache-busting mechanism when users have custom favicon.ico files?
I was thinking of something like this:

if self._favicon:
    favicon_url = self.get_asset_url(self._favicon)
    favicon_path = os.path.join(self._assets_folder, favicon_url)
    favicon_changed_time = os.path.getmtime(favicon_path)
    favicon_url += '?v={}'.format(favicon_changed_time)
else:
    favicon_url = '{}_favicon'.format(self.config.requests_pathname_prefix)
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(favicon_url)

This should bust the cache every time the favicon.ico file is modified. Got the idea from here

I'm 💃 with or without this change, but I think this will help save people a lot of time who can't figure out why their favicon.ico won't change.

[rmarren1]

Looks good, tested it with a bunch of different favicons. Would be nice to have cache busting if it works.

[T4rk1n]

@rmarren1 good point, I will add cache busting for the assets favicon.