Enable extensions to provide a default settings dictionary

Review Request #2907 — Created Feb. 20, 2012 and submitted

smacleod
Djblets
djblets
When an extension is loaded for the first time it is initialized with an empty settings dictionary. All extension authors must either initialize the settings dictionary and save it, or check at use if the setting exists.

This modification allows an extension author to specify a default settings dictionary, which will be used if a key is not present in extension.settings. Instead of copying all defaults, I only reference them when a missing key is accessed. Another option would be to initialize the settings when the extension is loaded. Feel free to suggest other ideas.
Tested in local dev with dummy extensions. Ran djblets unit tests.
Description From Last Updated

This chunk of code could probably be refactored. Usually you don't want to explicitly return a boolean, but rather do ...

ME medanat

I'd prefer "" instead of []. To make it more readable code-wise with "", you can use '' for the ...

chipx86chipx86

Each dict key should be on its own line: "" % { 'key': key, 'ext': self.extension.id, }

chipx86chipx86

If you make the default for default_settings be {} instead, this can just be: return key in self.extension.default_settings You can ...

chipx86chipx86
ME
  1. Looking good, just two minor issues (for me at least).
  2. djblets/extensions/base.py (Diff revision 1)
     
     
    I feel this is the wrong place for this import, I may be wrong.
    1. KeyError should always be in the global namespace
  3. djblets/extensions/base.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This chunk of code could probably be refactored. Usually you don't want to explicitly return a boolean, but rather do something similar to this:
    
    return super(Settings, self).__contains__(key)
    
    (I know this isn't a solution in your case, but you should try something similar) 
    1. I thought about this for a while and I can't come up with anything which doesn't explicitly return a boolean. Anyone else have an idea?
    2. You can combine them all:
      
      return (super(Settings, self).__contains__(key) or
              (self.extension.default_settings is not None and
               key in self.extension.default_settings))
      
      Although I'm not sure it's any clearer.
    3. Yeah, that isn't the most readable. Although combining the second set of conditions may work:
      
      if super(Settings, self).__contatins__(key):
          return True
      
      return (self.extension.default_settings is not None and
              key in self.extension.default_settings)
    4. That seems like a good compromise.
  4. 
      
SM
chipx86
  1. 
      
  2. djblets/extensions/base.py (Diff revision 2)
     
     
    I'd prefer "" instead of [].
    
    To make it more readable code-wise with "", you can use '' for the string itself.
  3. djblets/extensions/base.py (Diff revision 2)
     
     
    Each dict key should be on its own line:
    
    "" % {
        'key': key,
        'ext': self.extension.id,
    }
  4. djblets/extensions/base.py (Diff revision 2)
     
     
     
    If you make the default for default_settings be {} instead, this can just be:
    
        return key in self.extension.default_settings
    
    You can also simplify the logic in __getitem__.
  5. 
      
SM
chipx86
  1. Ship It!
  2. 
      
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (21192d0)
Loading...