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

Add support for default favicon verification disabling #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

advance512
Copy link

  • Added option for disabling verification by HTTP request of default /favicon.ico file, to have similar behavior
    to tag-based favicon results.
  • Fixed flaky test.

Includes PR #32 as well, as it wasn't merged yet.

Fixed issue with dimensions(tag) not working for some websites
Change: test corrected and minimized
…/favicon.ico file, to have similar behavior

  to tag-based favicon results.
* Fixed flaky test.
@advance512
Copy link
Author

@scottwernervt any thoughts about merging this in?

Copy link
Owner

@scottwernervt scottwernervt left a comment

Choose a reason for hiding this comment

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

I do not want to break Python 2 support even though it is deprecated (a lot of people still use it). I was thinking this week that maybe we should refactor the package and make each parser method public:

  • def get(url, *args, **request_kwargs) uses parses below
  • def parse_default_icon(url, **request_kwargs)
  • def parse_html_icons(url, html)

This would handle your case of parsing icons with your own html content instead of relying on bs4. Once you remove verify_default_icon arg, I will merge your PR, refactor it to above, and make a new release.

the `url`.
:type html_override: str or None

:param verify_default_icon: Whether to verify the existence of the default
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove verify_default_icon arg and logic. I do not understand why a user would want to return a found icon http://www.mysite.com/favicon.ico that doesn't actually exist?

@advance512
Copy link
Author

Why don't we increase the major version, so it can include breaking changes? We can version it v1.0.0.

Python 2 has reached its end of life. There is no point in supporting it in new versions of this project.
The older versions of favicon (0.x) will still support Python 2, and bug fixes can be released in minor version updates.

What do you think?

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.

2 participants