Revamp avatar service registry API

Review Request #8479 — Created Oct. 20, 2016 and submitted

Information

Djblets
release-0.10.x

Reviewers

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.

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/ ?

daviddavid

All the arguments could go on the same line.

daviddavid

I think the old is_configurable property was nicer API here.

daviddavid

Mind putting this into an else clause?

daviddavid

Hm. Do we want to keep a cache of instantiated services?

daviddavid

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

daviddavid

redefinition of unused 'unregister' from line 135

reviewbotreviewbot

redefinition of unused 'unregister' from line 135

reviewbotreviewbot

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

daviddavid

These should be absolute class paths.

chipx86chipx86

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

daviddavid

Since the else clause returns, how about: if service is None: return None if not self.is_enabled(service): ... ...

chipx86chipx86

type here and below (sorry!)

chipx86chipx86

No blank line.

chipx86chipx86

This can be: configuration = self._settings_manager_class(user).configuration_for( self.avatar_service_id) It fits in the line.

chipx86chipx86

"subclassed"

chipx86chipx86

"URLs"

chipx86chipx86
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. djblets/avatars/forms.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    All the arguments could go on the same line.

  3. djblets/avatars/forms.py (Diff revision 2)
     
     
    Show all issues

    I think the old is_configurable property was nicer API here.

    1. 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.

  4. djblets/avatars/registry.py (Diff revision 2)
     
     
     
     
    Show all issues

    Mind putting this into an else clause?

  5. djblets/avatars/registry.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    Hm. Do we want to keep a cache of instantiated services?

    1. That sounds good to me.

  6. djblets/avatars/registry.py (Diff revision 2)
     
     
    Show all issues

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

    1. 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).

  7. 
      
brennie
reviewbot
  1. 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
    
    
  2. djblets/avatars/registry.py (Diff revision 3)
     
     
    Show all issues
     redefinition of unused 'unregister' from line 135
    
  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. djblets/avatars/registry.py (Diff revision 4)
     
     
    Show all issues
     redefinition of unused 'unregister' from line 135
    
  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
david
  1. 
      
  2. Show all issues

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

  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. djblets/avatars/forms.py (Diff revision 7)
     
     
    Show all issues

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

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

  3. djblets/avatars/registry.py (Diff revision 7)
     
     
    Show all issues

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

    1. 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.

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

  4. 
      
chipx86
  1. 
      
  2. djblets/avatars/registry.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    These should be absolute class paths.

  3. djblets/avatars/registry.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since the else clause returns, how about:

    if service is None:
        return None
    
    if not self.is_enabled(service):
        ...
    
    ...
    
  4. djblets/avatars/registry.py (Diff revision 7)
     
     
    Show all issues

    type here and below (sorry!)

    1. I swore I fixed all these :(

  5. djblets/avatars/services/base.py (Diff revision 7)
     
     
     
     
    Show all issues

    No blank line.

  6. djblets/avatars/services/base.py (Diff revision 7)
     
     
     
    Show all issues

    This can be:

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

    It fits in the line.

  7. djblets/avatars/settings.py (Diff revision 7)
     
     
    Show all issues

    "subclassed"

  8. djblets/avatars/tests.py (Diff revision 7)
     
     
    Show all issues

    "URLs"

  9. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.10.x (fc23582)