Revamp avatar service registry API

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

Barret Rennie
Djblets
release-0.10.x
8480, 8481
djblets

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.

  • 7
  • 0
  • 9
  • 1
  • 17
Description From Last Updated
These should be absolute class paths. Christian Hammond Christian Hammond
Since the else clause returns, how about: if service is None: return None if not self.is_enabled(service): ... ... Christian Hammond Christian Hammond
type here and below (sorry!) Christian Hammond Christian Hammond
No blank line. Christian Hammond Christian Hammond
This can be: configuration = self._settings_manager_class(user).configuration_for( self.avatar_service_id) It fits in the line. Christian Hammond Christian Hammond
"subclassed" Christian Hammond Christian Hammond
"URLs" Christian Hammond Christian Hammond
Review Bot
  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. 
      
Barret Rennie
Review Bot
  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 Trowbridge
  1. 
      
  2. djblets/avatars/forms.py (Diff revision 2)
     
     
     
     
     
     

    All the arguments could go on the same line.

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

    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)
     
     
     
     

    Mind putting this into an else clause?

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

    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)
     
     

    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. 
      
Barret Rennie
Review Bot
  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)
     
     
     redefinition of unused 'unregister' from line 135
    
  3. 
      
Barret Rennie
Review Bot
  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)
     
     
     redefinition of unused 'unregister' from line 135
    
  3. 
      
Barret Rennie
Review Bot
  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 Trowbridge
  1. Ship It!
  2. 
      
David Trowbridge
  1. 
      
  2. Actually, wait. Did you update this to bring back has_service for https://reviews.reviewboard.org/r/8480/diff/1-2/ ?

  3. 
      
Barret Rennie
Review Bot
  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. 
      
Barret Rennie
Review Bot
  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 Trowbridge
  1. 
      
  2. djblets/avatars/forms.py (Diff revision 7)
     
     

    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)
     
     

    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. 
      
Christian Hammond
  1. 
      
  2. djblets/avatars/registry.py (Diff revision 7)
     
     
     
     
     
     

    These should be absolute class paths.

  3. 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):
        ...
    
    ...
    
  4. djblets/avatars/registry.py (Diff revision 7)
     
     

    type here and below (sorry!)

    1. I swore I fixed all these :(

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

    No blank line.

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

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

    "subclassed"

  8. djblets/avatars/tests.py (Diff revision 7)
     
     
  9. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (fc23582)
Loading...