Summary

Enable extensions to provide a default settings dictionary

Review Request #2907 — Created Feb. 20, 2012 and submitted March 10, 2012, 6 a.m.

Information

Owner

smacleod

Repository

Djblets

Branch

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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.

Testing Done

Tested in local dev with dummy extensions. Ran djblets unit tests.

Issues

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	Feb. 26, 2012, 6:31 a.m.
I'd prefer "" instead of []. To make it more readable code-wise with "", you can use '' for the …	chipx86	March 3, 2012, 7:37 a.m.
Each dict key should be on its own line: "" % { 'key': key, 'ext': self.extension.id, }	chipx86	March 3, 2012, 7:40 a.m.
If you make the default for default_settings be {} instead, this can just be: return key in self.extension.default_settings You can …	chipx86	March 3, 2012, 7:56 a.m.

Looking good, just two minor issues (for me at least).

djblets/extensions/base.py (Diff revision 1)

I feel this is the wrong place for this import, I may be wrong.

david Feb. 25, 2012, 7:27 a.m.

KeyError should always be in the global namespace

djblets/extensions/base.py (Diff revision 1)

The issue has been resolved. Show all issues

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)

smacleod Feb. 25, 2012, 2:25 a.m.

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?

david Feb. 25, 2012, 7:12 a.m.

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.

smacleod Feb. 25, 2012, 7:23 a.m.

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)

david Feb. 25, 2012, 7:27 a.m.
```
That seems like a good compromise.
```

Change Summary:

Updated based on review

Diff:

Revision 2 (+37)

Show changes

djblets/extensions/base.py

djblets/extensions/base.py (Diff revision 2)

The issue has been resolved. Show all issues

I'd prefer "" instead of [].

To make it more readable code-wise with "", you can use '' for the string itself.

djblets/extensions/base.py (Diff revision 2)

The issue has been resolved. Show all issues

Each dict key should be on its own line:

"" % {
    'key': key,
    'ext': self.extension.id,
}

djblets/extensions/base.py (Diff revision 2)

The issue has been resolved. Show all issues

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

Change Summary:

Updated based on review

Diff:

Revision 3 (+37)

Show changes

djblets/extensions/base.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (21192d0)