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

Default https #9

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Default https #9

merged 6 commits into from
Apr 9, 2021

Conversation

xurble
Copy link
Contributor

@xurble xurble commented Apr 6, 2021

Gravatar has changed so that it now recommends the https:// urls as the default

The PR reverses the default of use_ssl to be False

It also makes the domain always be www.gravatar.com since that seems to be the modern recommendation, they don't mention secure.gravatar.com any more.

https://en.gravatar.com/site/implement/images/

Finally at adds a use_ssl (default False) parameter to the get_profile method.

@pabluk
Copy link
Owner

pabluk commented Apr 8, 2021

Hi @xurble, thanks for your contribution! it makes sense at this time where everything is being HTTPS by default. I'll take a look tomorrow to review this changes and prepare a new release because changing the default value of use_ssl=True could be a breaking change for some users.

@pabluk pabluk self-requested a review April 8, 2021 14:46
@xurble
Copy link
Contributor Author

xurble commented Apr 8, 2021

You should take the plunge and release a 1.0 😀

In all seriousness, though it's been a really useful library for me. Thank you.

Copy link
Owner

@pabluk pabluk left a comment

Choose a reason for hiding this comment

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

Everything looks very good and tested! Just one question regarding the changes on README.md

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
libgravatar/__init__.py Show resolved Hide resolved
@pabluk
Copy link
Owner

pabluk commented Apr 9, 2021

By the way, when reviewing this PR I spotted an old issue, doctest strings aren't being tested, so I opened and merged this PR to fix it.

@xurble if you could rebase this branch from master and also update the protocol (https) into the following docstrings in order to include those tests, it would be great.

@pabluk
Copy link
Owner

pabluk commented Apr 9, 2021

You should take the plunge and release a 1.0

It would be probably the case 😅

@xurble
Copy link
Contributor Author

xurble commented Apr 9, 2021

OK, I am terrible with pull requests. I think that this is now up to date with your doc string test change and I've fixed the docstrings and the readme to be correct.

@pabluk
Copy link
Owner

pabluk commented Apr 9, 2021

Everything looks good and ready 👍 I'll publish a new release asap.

@xurble thank you very much for your time and this great enhancement!

@pabluk pabluk merged commit 16d7bda into pabluk:master Apr 9, 2021
@pabluk
Copy link
Owner

pabluk commented Apr 9, 2021

The new release 1.0.0 is now available on PyPI https://pypi.org/project/libgravatar/ 🚀

@xurble xurble deleted the default_https branch April 12, 2021 20:06
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