Summary

No longer require an HttpRequest for avatars

Review Request #8165 — Created May 16, 2016 and submitted May 18, 2016, 3:57 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Commit

25d8d42...

Reviewers

Groups

djblets

People

Description

Avatar services no longer require an HttpRequest to get the uncached
avatar URLs for a user. Previously, the Gravatar avatar service used the
HttpRequest to determine whether or not HTTPS should be used for the
resulting URL. Now, we always use an HTTPS url as there is no reason not
to.

The unit tests have been udpated to reflect these changes.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
This is going to break backwards-compatibility. We We should keep the parameter btu work without it, make it None by …	chipx86	May 16, 2016, 5:18 p.m.
Should say "optional".	chipx86	May 16, 2016, 5:18 p.m.
Typo.	chipx86	May 16, 2016, 5:18 p.m.
No opening paren here.	chipx86	May 16, 2016, 5:18 p.m.
Should specify "optional."	chipx86	May 16, 2016, 5:05 p.m.
Should specify if this is in pixels.	chipx86	May 16, 2016, 5:18 p.m.
Typo.	chipx86	May 16, 2016, 5:18 p.m.
No opening paren.	chipx86	May 16, 2016, 5:18 p.m.
Should specify optional.	chipx86	May 16, 2016, 5:05 p.m.
Should specify that this is in pixels.	chipx86	May 16, 2016, 5:19 p.m.
Can we put parens around the computation? It'll help a bit with readability.	chipx86	May 16, 2016, 6:07 p.m.
Missing "optional".	chipx86	May 16, 2016, 5:51 p.m.
We can start the string on the warn( line. It's only one more space.	chipx86	May 16, 2016, 5:53 p.m.
Let's reference the argument name, like: 'The "email" argument cannot be None.' Might be worth just checking for truthiness, and …	chipx86	May 16, 2016, 6:06 p.m.
Missing "optional".	chipx86	May 16, 2016, 5:51 p.m.
Same comments as above.	chipx86	May 16, 2016, 5:53 p.m.

Tool: Pyflakes
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst

Ship it!

```
Ship It!
```

djblets/gravatars/__init__.py (Diff revision 1)

Show all issues

This is going to break backwards-compatibility. We We should keep the parameter btu work without it, make it None by default.

chipx86 May 16, 2016, 4:06 p.m.
```
Same with the other functions below.
```

chipx86 May 16, 2016, 4:09 p.m.

Maybe not make it None, because that impacts the other parameters... Kind of a crumby situation. We'll just have to take the request, allow it to be None, and really just ignore it :/

chipx86 May 16, 2016, 4:12 p.m.

I think what we do is make them all None, document that the arguments should be provided as keyword arguments, and raise a ValueError if the e-mail address isn't provided.

david May 16, 2016, 4:17 p.m.

This API hasn't officially shipped yet. Do we really care enough about backwards compatibility to jump through these hoops?

chipx86 May 16, 2016, 4:18 p.m.

These gravatar functions have. They're used in RB 1.7+.

djblets/gravatars/__init__.py (Diff revision 1)
Show all issues
```
Should say "optional".
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Typo.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
No opening paren here.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Should specify "optional."
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Should specify if this is in pixels.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Typo.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
No opening paren.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Should specify optional.
```
djblets/gravatars/templatetags/gravatars.py (Diff revision 1)
Show all issues
```
Should specify that this is in pixels.
```

Change Summary:

Addressed Christian's issues

Diff:

Revision 2 (+137 -95)

Show changes

	djblets/avatars/tests.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/gravatars/__init__.py
	djblets/gravatars/templatetags/gravatars.py
	djblets/gravatars/templatetags/tests.py
	docs/djblets/guides/avatars/writing-avatar-services.rst

Tool: PEP8 Style Checker
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst



Tool: Pyflakes
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst

djblets/avatars/services/gravatar.py (Diff revision 2)

Show all issues

Can we put parens around the computation? It'll help a bit with readability.

djblets/gravatars/__init__.py (Diff revision 2)

Show all issues

Missing "optional".

brennie May 16, 2016, 5:53 p.m.

We dont actually have a fork of Napoleon that supports this syntax. I'd rather have it in the description of the argument, or use Optional[int].

chipx86 May 16, 2016, 5:53 p.m.

We're using it everywhere else, though. Consistency is good right now. The fork will support it.

djblets/gravatars/__init__.py (Diff revision 2)

Show all issues

We can start the string on the warn( line. It's only one more space.

djblets/gravatars/__init__.py (Diff revision 2)

Show all issues

Let's reference the argument name, like: 'The "email" argument cannot be None.'

Might be worth just checking for truthiness, and saying "cannot be None or an empty string."

djblets/gravatars/__init__.py (Diff revision 2)
Show all issues
```
Missing "optional".
```
djblets/gravatars/__init__.py (Diff revision 2)
Show all issues
```
Same comments as above.
```

Change Summary:

Addressed Christian's issues

Diff:

Revision 3 (+135 -95)

Show changes

	djblets/avatars/tests.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/gravatars/__init__.py
	djblets/gravatars/templatetags/gravatars.py
	djblets/gravatars/templatetags/tests.py
	docs/djblets/guides/avatars/writing-avatar-services.rst

Tool: Pyflakes
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/gravatars/templatetags/tests.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/gravatars/templatetags/gravatars.py
    djblets/avatars/tests.py
    djblets/gravatars/__init__.py

Ignored Files:
    docs/djblets/guides/avatars/writing-avatar-services.rst

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (dd36743)