Summary

Revamp avatar service registry API

Review Request #8479 — Created Oct. 20, 2016 and submitted Oct. 25, 2016, 9:47 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Blocks

8480, 8481

Reviewers

Groups

djblets

People

Description

AvatarServices now require an AvatarSettingsManager class when

being instantiated. Previously, this was an optional argument that

defaulted to the default AvatarSettingsManager which throws

NotImplementedError for all its methods. However, registries were not

guaranteed to return any third party avatar services instantiated with

the correct settings manager.
This has been corrected by making the avatar services registry always

store instances instead. It has simplified the API as now most things

can be done via method calls with the AvatarService class as an

argument instead of its avatar_service_id attribute. It also has the

benefit of making unit tests more succinct.
The AvatarServicesRegistry's get_avatar_service, for_user, and

configurable_services nethods have been re-written to return an

instance of the requested avatar service, instantiated with the

correct settings manager class.
Unit tests & the rest of the avatar services suite have been

re-written in light of these changes.

Testing Done

Ran unit tests.
Tested avatar services still work with Review Board.

Issues

Description	From	Last Updated
Actually, wait. Did you update this to bring back has_service for https://reviews.reviewboard.org/r/8480/diff/1-2/ ?	david	Oct. 21, 2016, 11:35 p.m.
All the arguments could go on the same line.	david	Oct. 21, 2016, 8:03 p.m.
I think the old is_configurable property was nicer API here.	david	Oct. 21, 2016, 8:02 p.m.
Mind putting this into an else clause?	david	Oct. 21, 2016, 8:04 p.m.
Hm. Do we want to keep a cache of instantiated services?	david	Oct. 21, 2016, 8:15 p.m.
We don't use this syntax anywhere else, do we?	david	Oct. 24, 2016, 9:52 a.m.
redefinition of unused 'unregister' from line 135	reviewbot	Oct. 21, 2016, 8:30 p.m.
redefinition of unused 'unregister' from line 135	reviewbot	Oct. 21, 2016, 8:31 p.m.
Can this be falsy but not None? Would it be more correct to do if avatar_service is None?	david	Oct. 24, 2016, 4:19 p.m.
These should be absolute class paths.	chipx86	Oct. 25, 2016, 2:08 a.m.
Can we add an else clause that asserts that the service is present and enabled?	david	Oct. 24, 2016, 4:16 p.m.
Since the else clause returns, how about: if service is None: return None if not self.is_enabled(service): ... ...	chipx86	Oct. 25, 2016, 2:08 a.m.
type here and below (sorry!)	chipx86	Oct. 25, 2016, 2:08 a.m.
No blank line.	chipx86	Oct. 25, 2016, 2:08 a.m.
This can be: configuration = self._settings_manager_class(user).configuration_for( self.avatar_service_id) It fits in the line.	chipx86	Oct. 25, 2016, 2:08 a.m.
"subclassed"	chipx86	Oct. 25, 2016, 2:08 a.m.
"URLs"	chipx86	Oct. 25, 2016, 2:08 a.m.

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



Tool: Pyflakes
Processed Files:
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

Description:

		`AvatarService`s now require an `AvatarSettingsManager` class when
		being instantiated. Previously, this was an optional argument that
		defaulted to the default `AvatarSettingsManager` which throws
		`NotImplementedError` for all its methods. However, registries were not
		guaranteed to return any third party avatar services instantiated with
		the correct settings manager.

		This has been corrected by making the avatar services registry always
		store instances instead. It has simplified the API as now most things
		can be done via method calls with the `AvatarService` class as an
		argument instead of its `avatar_service_id` attribute. It also has the
		benefit of making unit tests more succinct.

~		The `AvatarServicesRegistry`'s `get_avatar_service` and `for_user`
~		methods have been re-written to return an instance of the requested
~		avatar service, instantiated with the correct settings manager class.
	~	The `AvatarServicesRegistry`'s `get_avatar_service`, `for_user`, and
	~	`configurable_services` nethods have been re-written to return an
	~	instance of the requested avatar service, instantiated with the
	+	correct settings manager class.

~		Unit tests have been re-written in light of these changes.
	~	Unit tests & the rest of the avatar services suite have been
	+	re-written in light of these changes.

Testing Done:

~		Ran unit tests.
	~	Ran unit tests.
	+	Tested avatar services still work with Review Board.

Diff:

Revision 2 (+217 -188)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

djblets/avatars/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
All the arguments could go on the same line.
```

djblets/avatars/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

I think the old is_configurable property was nicer API here.

brennie Oct. 21, 2016, 5:57 p.m.

The former is not accesible as a property on a class (classmethod properties aren't really a thing), and here avatar_service is the class, not the instantiated object.

djblets/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Mind putting this into an else clause?
```
djblets/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Hm. Do we want to keep a cache of instantiated services?
```
1. brennie Oct. 21, 2016, 5:57 p.m.
  That sounds good to me.

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

The issue has been dropped. Show all issues

We don't use this syntax anywhere else, do we?

brennie Oct. 21, 2016, 5:57 p.m.

No, but type is not specific enough. This is standard PEP 484 syntax for denoting isinstance(service, AvatarService). My editor really dislikes it when we use type for this (and for that matter, object instead of Any).

I really think the benefits of PEP 484 outweigh the cons. Sphinx is getting support for it with Napoleon (the docstring format we use) in its next major release (it already supports it if you use Napoleon from sphinx-contrib).

Diff:

Revision 3 (+250 -188)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

djblets/avatars/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 redefinition of unused 'unregister' from line 135
```

Diff:

Revision 4 (+250 -188)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

djblets/avatars/registry.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 redefinition of unused 'unregister' from line 135
```

Diff:

Revision 5 (+236 -188)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

Actually, wait. Did you update this to bring back has_service for https://reviews.reviewboard.org/r/8480/diff/1-2/ ?

Diff:

Revision 6 (+254 -189)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

Diff:

Revision 7 (+250 -189)

Show changes

	djblets/avatars/forms.py
	djblets/avatars/registry.py
	djblets/avatars/settings.py
	djblets/avatars/tests.py
	djblets/avatars/services/base.py

Tool: Pyflakes
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/avatars/forms.py
    djblets/avatars/settings.py
    djblets/avatars/tests.py
    djblets/avatars/registry.py
    djblets/avatars/services/base.py

djblets/avatars/forms.py (Diff revision 7)

The issue has been resolved. Show all issues

Can this be falsy but not None? Would it be more correct to do if avatar_service is None?

brennie Oct. 24, 2016, 1:55 p.m.

Not unless someone defines __bool__ as a class method on the class, but I will change it.

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

The issue has been resolved. Show all issues

Can we add an else clause that asserts that the service is present and enabled?

brennie Oct. 24, 2016, 1:55 p.m.

We check if it is disabled on line 136. If it is not present in the registry, we are always going to enter the body of this if statement becuase we will enver set the entry in the cache for a non-existant entry.

Also, we remove entries from the cache when they become disabled and unregistered.

david Oct. 24, 2016, 1:57 p.m.

That's why I'm suggesting an assertion rather than a condition.

Fix it!

djblets/avatars/registry.py (Diff revision 7)
An issue was opened. Show all issues
```
These should be absolute class paths.
```

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

An issue was opened. Show all issues

Since the else clause returns, how about:

if service is None:
    return None

if not self.is_enabled(service):
    ...

...

djblets/avatars/registry.py (Diff revision 7)
An issue was opened. Show all issues
```
type here and below (sorry!)
```
1. brennie Oct. 25, 2016, 6:47 p.m.
  I swore I fixed all these :(
djblets/avatars/services/base.py (Diff revision 7)
An issue was opened. Show all issues
```
No blank line.
```

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

An issue was opened. Show all issues

This can be:

configuration = self._settings_manager_class(user).configuration_for(
    self.avatar_service_id)


It fits in the line.

djblets/avatars/settings.py (Diff revision 7)
An issue was opened. Show all issues
```
"subclassed"
```
djblets/avatars/tests.py (Diff revision 7)
An issue was opened. Show all issues
```
"URLs"
```

Status:: Completed
Change Summary:: Pushed to release-0.10.x (fc23582)