Summary

Add an Avatar Service Registry to Djblets

Review Request #7844 — Created Jan. 6, 2016 and submitted Feb. 22, 2016, 7:11 a.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

~~7827~~

Blocks

7809

Commit

5146fb6...

Reviewers

Groups

djblets

People

Description

The avatar services registry is a new object for managing avatar
services, which are defined to provide multiple ways of displaying
users' avatars. Currently, the only supported service is the Gravatar
(https://gravatar.com) service.

Avatars support the srcset attribute for <img> tags. In browsers
where it is not supported, a polyfill is used.

Testing Done


Ran unit tests
Used this with an upcoming patch in Review Board.
Tested the srcset polyfill in IE11 with Review Board.

Issues

Description	From	Last Updated
Col: 1 W391 blank line at end of file	reviewbot	Jan. 7, 2016, 2:23 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 7, 2016, 2:23 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 7, 2016, 2:23 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 7, 2016, 2:23 p.m.
Col: 1 E303 too many blank lines (3)	reviewbot	Jan. 7, 2016, 2:31 p.m.
The suspense is killing me.	david	Jan. 12, 2016, 11:23 p.m.
Blank line after this.	david	Jan. 12, 2016, 11:23 p.m.
Blank line after this.	david	Jan. 12, 2016, 11:23 p.m.
Missing the "Yields" label.	david	Jan. 12, 2016, 11:23 p.m.
Missing "Raises"	david	Jan. 12, 2016, 11:23 p.m.
Should we include 3x for future proofing?	david	Jan. 12, 2016, 11:23 p.m.
This is wordy. Can we drop the first "services"? Maybe just "avatars_enabled_services" and "avatars_default_service"?	chipx86	Jan. 15, 2016, 1:05 p.m.
The set shouldn't be there.	chipx86	Jan. 15, 2016, 1:06 p.m.
Given the complexity of this and the getter (more this one), and the note about having to call save manually, …	chipx86	Jan. 16, 2016, 9:09 a.m.
I feel that we should be using subclasses of this. Much of the implementation of registries should be an internal …	chipx86	Jan. 15, 2016, 1:38 p.m.
Needs the full path. Same with others below.	chipx86	Jan. 15, 2016, 1:38 p.m.
There's an extra "and the".	chipx86	Jan. 15, 2016, 1:40 p.m.
Instead of passing a default, we should fall back on what the app may have specified as the default when …	chipx86	Jan. 15, 2016, 1:40 p.m.
No blank line.	chipx86	Jan. 15, 2016, 1:40 p.m.
If we make use of siteconfig defaults, this information can be encoded there, rather than being hard-coded here.	chipx86	Jan. 15, 2016, 1:40 p.m.
"human-readable"	chipx86	Jan. 15, 2016, 1:41 p.m.
This sounds like the template will be rendering an image, but it's the HTML tags for an image.	chipx86	Jan. 15, 2016, 1:41 p.m.
Extra "the"	chipx86	Jan. 15, 2016, 1:41 p.m.
Alignment issue.	chipx86	Jan. 15, 2016, 1:41 p.m.
A more efficient way of doing this would be: try: urls = request._avatar_cache[key] except KeyError: urls = self.get_blah(...) request._avatar_cache[key] = …	chipx86	Jan. 15, 2016, 1:42 p.m.
Alignment issue.	chipx86	Jan. 15, 2016, 1:42 p.m.
Let's change this to: raise NotImplementedError( '%r must implement get_avatar_urls_uncached().' % self.__class__ ) That way, when there are lots spewing …	chipx86	Jan. 15, 2016, 1:43 p.m.
80 is the default for gravatars, but we care more about the caller. The caller should expect consistency in sizes, …	chipx86	Jan. 15, 2016, 1:44 p.m.
We could support all ##x image sizes by doing: <img {% for key, url in urls.items %}{% if key\|startswith:"@" %}{% …	chipx86	Jan. 16, 2016, 8:57 a.m.
We should probably raise a ValueError if fmt is empty, instead of asserting, and specify the class and error name, …	chipx86	Jan. 15, 2016, 1:54 p.m.
'mark_safe' imported but unused	reviewbot	Jan. 29, 2016, 8:32 a.m.
Description on the next line.	chipx86	Feb. 19, 2016, 4:02 a.m.
The else is implied, so you can remove it. The first part is a guard, and doesn't have to be …	chipx86	Feb. 19, 2016, 4:02 a.m.
You can remove the blank line here.	chipx86	Feb. 19, 2016, 4:02 a.m.
This is already saved below. Also, are there cases where we don't need to save?	chipx86	Feb. 19, 2016, 4:07 a.m.
Thinking this should be avatar_id. Some of our older stuff uses id (which is a reserved word), and most of …	chipx86	Feb. 19, 2016, 4:07 a.m.
This is going to be a SafeText.	chipx86	Feb. 19, 2016, 4:12 a.m.
Blank line not needed.	chipx86	Feb. 19, 2016, 4:13 a.m.
Having a hard time parsing this. Maybe "The keyword arguments passed to the error-specific formatting string."	chipx86	Feb. 19, 2016, 4:13 a.m.
No need for None.	chipx86	Feb. 19, 2016, 4:14 a.m.
Let's include the class name in this (using __class__).	chipx86	Feb. 19, 2016, 4:14 a.m.
No blank line.	chipx86	Feb. 19, 2016, 4:19 a.m.
We shouldn't assume anything about the safeness of the URLs. If the URL has a & in it, then it'll …	chipx86	Feb. 21, 2016, 12:12 a.m.

Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

djblets/avatars/errors.py (Diff revision 1)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

djblets/avatars/tests.py (Diff revision 1)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

djblets/avatars/tests.py (Diff revision 1)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

djblets/avatars/tests.py (Diff revision 1)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

Change Summary:

Fix tests; PEP8

Diff:

Revision 2 (+981 -12)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	3 more

Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

djblets/avatars/tests.py (Diff revision 2)
Show all issues
```
Col: 1
 E303 too many blank lines (3)
```

Change Summary:

PEP8

Diff:

Revision 3 (+980 -12)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	3 more

Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

djblets/avatars/errors.py (Diff revision 3)
Show all issues
```
The suspense is killing me.
```
djblets/avatars/registry.py (Diff revision 3)
Show all issues
```
Blank line after this.
```
djblets/avatars/registry.py (Diff revision 3)
Show all issues
```
Blank line after this.
```
djblets/avatars/registry.py (Diff revision 3)
Show all issues
```
Missing the "Yields" label.
```
djblets/avatars/registry.py (Diff revision 3)
Show all issues
```
Missing "Raises"
```

djblets/avatars/services/base.py (Diff revision 3)

Show all issues

Should we include 3x for future proofing?

brennie Jan. 12, 2016, 3 a.m.

If we do, we should say it may contain a 3x key. I suppose I should update the JS as well to support 3x avatars.

Change Summary:

Addressed David's issues.

Commit:

+	5146fb6b4e9a1d9b85c9bbf09facc8a57a388aeb

Diff:

Revision 4 (+1001 -12)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	3 more

Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

djblets/avatars/registry.py (Diff revision 4)

Show all issues

This is wordy. Can we drop the first "services"? Maybe just "avatars_enabled_services" and "avatars_default_service"?

djblets/avatars/registry.py (Diff revision 4)
Show all issues
```
The set shouldn't be there.
```

djblets/avatars/registry.py (Diff revision 4)

Show all issues

Given the complexity of this and the getter (more this one), and the note about having to call save manually, and how we have similar functions that have save as a built-in flag, I think maybe we should just use standard methods for this.

Properties are great when it's a trivial thing, like taking a value and stuffing it in some nested object, or normalizing the value. A caller expects that an assignment like that will basically just work. This one is a lot more complicated, with the possibility of other exceptions being raised and the other requirements placed on the caller.

djblets/avatars/registry.py (Diff revision 4)

Show all issues

I feel that we should be using subclasses of this. Much of the implementation of registries should be an internal detail, and not something that the caller should have to know much about. Right now, as a caller, I have to learn not just about the avatar service, but I also have to dive into the registry methods.
What about extending registries to have a class pointing to the exceptions that should be used? Then there could be an AvatarNotFoundError that's raised, subclassing ItemLookupError, when raised either by this subclass or by the base class.
Such an error would have a lot more meaning to a user, and to anyone reading or contributing to the code.

djblets/avatars/registry.py (Diff revision 4)
Show all issues
```
Needs the full path. Same with others below.
```
djblets/avatars/registry.py (Diff revision 4)
Show all issues
```
There's an extra "and the".
```

djblets/avatars/registry.py (Diff revision 4)

Show all issues

Instead of passing a default, we should fall back on what the app may have specified as the default when registering settings. That way, we get that value even if not in the database.

djblets/avatars/registry.py (Diff revision 4)
Show all issues
```
No blank line.
```

djblets/avatars/registry.py (Diff revision 4)

Show all issues

If we make use of siteconfig defaults, this information can be encoded there, rather than being hard-coded here.

brennie Jan. 15, 2016, 2:12 p.m.

It ends up being hardcoded somewhere. I'd rather it be hardcoded in the get_defaults method because thats where registry defaults come from elsewhere.

djblets/avatars/services/base.py (Diff revision 4)
Show all issues
```
"human-readable"
```

djblets/avatars/services/base.py (Diff revision 4)

Show all issues

This sounds like the template will be rendering an image, but it's the HTML tags for an image.

djblets/avatars/services/base.py (Diff revision 4)
Show all issues
```
Extra "the"
```
djblets/avatars/services/base.py (Diff revision 4)
Show all issues
```
Alignment issue.
```

djblets/avatars/services/base.py (Diff revision 4)

Show all issues

A more efficient way of doing this would be:

try:
    urls = request._avatar_cache[key]
except KeyError:
    urls = self.get_blah(...)
    request._avatar_cache[key] = urls

djblets/avatars/services/base.py (Diff revision 4)
Show all issues
```
Alignment issue.
```

djblets/avatars/services/base.py (Diff revision 4)

Show all issues

Let's change this to:

raise NotImplementedError(
    '%r must implement get_avatar_urls_uncached().'
    % self.__class__
)


That way, when there are lots spewing errors, it's very clear what class is missing this definition.
(Also parens to be clear it's a method.)

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

Show all issues

80 is the default for gravatars, but we care more about the caller. The caller should expect consistency in sizes, regardless of what the defaults are in the backend if a size is unspecified.

Realistically, this value will almost surely never be used, because get_avatar_urls() is going to pass its own value.

I'd suggest having get_avatar_urls_uncached, both here and in AvatarService, simply not take a default value for size.

djblets/avatars/templates/avatars/avatar.html (Diff revision 4)

Show all issues

We could support all ##x image sizes by doing:

<img {% for key, url in urls.items %}{% if key|startswith:"@" %}{% if key == "@1x" %}src{% else %}{{key}}{% endif %}="{{url}}"{% endif %} />


However, <img srcset="..."/> and <picture/> are becoming things. These are neat because not only do they let you do "2x", "3x", etc., but also "512w" (for a 512 image width), which we could then allow in the list of URLs.
I think we could probably start trying to use this now (would need some preliminary testing), whether this change or another, and bonus points: We could just find a polyfill for it instead of writing and requiring use of $.retinaAvatar.
This could look like:

<img src="{{urls.1x}}" srcset="{% for size, url in urls.items %}{% spaceless %}
    {{url}}{% if size != "1x" %} {{size}}{% endif %}{% if not forloop.last %}, {% endif %}
{% endspaceless %}{% endfor %}" />


(I'm just leaving off the other attributes from these examples, but we'd obviously still want them.)

brennie Jan. 15, 2016, 2:12 p.m.

I'm going to update this to use srcset :)

djblets/registries/registry.py (Diff revision 4)

Show all issues

We should probably raise a ValueError if fmt is empty, instead of asserting, and specify the class and error name, so it's more clear what's going on if this is hit.

Change Summary:

Addressed Christian's issues.

Description:

		The avatar services registry is a new object for managing avatar
		services, which are defined to provide multiple ways of displaying
		users' avatars. Currently, the only supported service is the Gravatar
		(https://gravatar.com) service.

	+	Avatars support the `srcset` attribute for `<img>` tags. In browsers
	+	where it is not supported, a polyfill is used.
	+
		TODO: Docs still need to be added for this, but I want to get the API
		stabilized first before I write the docs.

Testing Done:

		Ran unit tests
		Used this with an upcoming patch in Review Board.
	+	Tested the `srcset` polyfill in IE11 with Review Board.

Diff:

Revision 5 (+1497 -23)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	7 more

Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/configforms/registry.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/configforms/tests.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/configforms/registry.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/configforms/tests.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

Ship it!

This looks good to me now. May want to get Christian to give it another look too, since it's pretty big.

Change Summary:

Remove the configforms stuff from this patch.
It was uploaded by accident.

Diff:

Revision 6 (+1093 -23)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	4 more

Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst



Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/services/__init__.py
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst

Change Summary:

Add docs

Diff:

Revision 7 (+1169 -25)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	8 more

Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst

djblets/avatars/services/gravatar.py (Diff revision 7)
Show all issues
```
 'mark_safe' imported but unused
```

Diff:

Revision 8 (+1169 -25)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	8 more

Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst



Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst

Description:

		The avatar services registry is a new object for managing avatar
		services, which are defined to provide multiple ways of displaying
		users' avatars. Currently, the only supported service is the Gravatar
		(https://gravatar.com) service.

		Avatars support the `srcset` attribute for `<img>` tags. In browsers
		where it is not supported, a polyfill is used.
-
-		TODO: Docs still need to be added for this, but I want to get the API
-		stabilized first before I write the docs.

Ship it!

```
Ship It!
```

```
Huh, was sure I published this...
```
djblets/avatars/registry.py (Diff revision 8)
Show all issues
```
Description on the next line.
```

djblets/avatars/registry.py (Diff revision 8)

Show all issues

The else is implied, so you can remove it. The first part is a guard, and doesn't have to be connected to the rest.

Same with ones below.

djblets/avatars/registry.py (Diff revision 8)
Show all issues
```
You can remove the blank line here.
```

djblets/avatars/registry.py (Diff revision 8)

Show all issues

This is already saved below.

Also, are there cases where we don't need to save?

djblets/avatars/services/base.py (Diff revision 8)

Show all issues

Thinking this should be avatar_id. Some of our older stuff uses id (which is a reserved word), and most of our newer stuff uses things like avatar_id.

djblets/avatars/services/base.py (Diff revision 8)
Show all issues
```
This is going to be a SafeText.
```
djblets/avatars/tests.py (Diff revision 8)
Show all issues
```
Blank line not needed.
```

djblets/registries/registry.py (Diff revision 8)

Show all issues

Having a hard time parsing this. Maybe "The keyword arguments passed to the error-specific formatting string."

djblets/registries/registry.py (Diff revision 8)
Show all issues
```
No need for None.
```

djblets/registries/registry.py (Diff revision 8)

Show all issues

Let's include the class name in this (using __class__).

djblets/static/djblets/js/jquery.gravy.retina.js (Diff revision 8)
Show all issues
```
No blank line.
```

djblets/util/templatetags/djblets_images.py (Diff revision 8)

Show all issues

We shouldn't assume anything about the safeness of the URLs. If the URL has a & in it, then it'll turn into an entity here. So instead, let's do:

return format_html_join(
    ', '
    '{0} {1}',
    (
        (url, descriptor)
        for descriptor, url in ...
        if url is ...
    ))

chipx86 Feb. 5, 2016, 3:33 p.m.

There's actually another place like this where we assume safety of the gravatar URL, whereas the old templates I don't think did that? I don't think we shouldn't assume safety of those URLs, but we should test that.

brennie Feb. 6, 2016, 1:31 a.m.

No they won't.

In [3]: from django.utils.html import mark_safe

In [4]: mark_safe('http://foo/y?a=b&c=d')

Out[4]: 'http://foo/y?a=b&c=d'


Also, documentation explicitly states that all URLs returned from avatar services must be marked safe text, so this should be fine.
Get back to me re: this and we can hash it out on Monday.

chipx86 Feb. 21, 2016, 12:12 a.m.

I really have no earthly idea what I was thinking when I wrote that. I don't think I read the code I thought I read.

Change Summary:

Addressed Christian's issues.

Diff:

Revision 9 (+1174 -25)

Show changes

	djblets/avatars/__init__.py
	djblets/avatars/errors.py
	djblets/avatars/registry.py
	djblets/avatars/tests.py
	djblets/avatars/services/__init__.py
	djblets/avatars/services/base.py
	djblets/avatars/services/gravatar.py
	djblets/avatars/templates/avatars/avatar.html
	8 more

Tool: PEP8 Style Checker
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst



Tool: Pyflakes
Processed Files:
    djblets/registries/registry.py
    djblets/util/templatetags/djblets_images.py
    djblets/avatars/services/gravatar.py
    djblets/avatars/services/base.py
    djblets/avatars/errors.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py

Ignored Files:
    djblets/avatars/templates/avatars/avatar.html
    djblets/static/djblets/js/jquery.gravy.retina.js
    docs/djblets/guides/index.rst
    djblets/avatars/services/__init__.py
    docs/djblets/guides/avatars/writing-avatar-services.rst
    djblets/avatars/__init__.py
    docs/djblets/coderef/index.rst
    docs/djblets/guides/avatars/index.rst
    docs/djblets/guides/registries/writing-registries.rst

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (d940248)