Revamp avatar service registry API
Review Request #8479 — Created Oct. 20, 2016 and submitted
AvatarService
s now require anAvatarSettingsManager
class when
being instantiated. Previously, this was an optional argument that
defaulted to the defaultAvatarSettingsManager
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 theAvatarService
class as an
argument instead of itsavatar_service_id
attribute. It also has the
benefit of making unit tests more succinct.The
AvatarServicesRegistry
'sget_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.
Ran unit tests.
Tested avatar services still work with Review Board.
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 | |
All the arguments could go on the same line. |
david | |
I think the old is_configurable property was nicer API here. |
david | |
Mind putting this into an else clause? |
david | |
Hm. Do we want to keep a cache of instantiated services? |
david | |
We don't use this syntax anywhere else, do we? |
david | |
redefinition of unused 'unregister' from line 135 |
reviewbot | |
redefinition of unused 'unregister' from line 135 |
reviewbot | |
Can this be falsy but not None? Would it be more correct to do if avatar_service is None? |
david | |
These should be absolute class paths. |
chipx86 | |
Can we add an else clause that asserts that the service is present and enabled? |
david | |
Since the else clause returns, how about: if service is None: return None if not self.is_enabled(service): ... ... |
chipx86 | |
type here and below (sorry!) |
chipx86 | |
No blank line. |
chipx86 | |
This can be: configuration = self._settings_manager_class(user).configuration_for( self.avatar_service_id) It fits in the line. |
chipx86 | |
"subclassed" |
chipx86 | |
"URLs" |
chipx86 |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+217 -188) |
-
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) I think the old
is_configurable
property was nicer API here. -
-
djblets/avatars/registry.py (Diff revision 2) Hm. Do we want to keep a cache of instantiated services?
-
-
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
-
-
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
-
-
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
-
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
-
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) Can this be falsy but not
None
? Would it be more correct to doif avatar_service is None
? -
djblets/avatars/registry.py (Diff revision 7) Can we add an
else
clause that asserts that the service is present and enabled?
-
-
-
djblets/avatars/registry.py (Diff revision 7) Since the
else
clause returns, how about:if service is None: return None if not self.is_enabled(service): ... ...
-
-
-
djblets/avatars/services/base.py (Diff revision 7) This can be:
configuration = self._settings_manager_class(user).configuration_for( self.avatar_service_id)
It fits in the line.
-
-