Add registries, which can be used to keep track of unique objects
Review Request #7780 — Created Nov. 25, 2015 and submitted
Registries are wrappers around Python's
setclass that provides the
additional constraints:
1. All registered members must have a core set of attributes
(defined per registry).
2. The values for these attributes must be unique across all members.A guide on writing registries has been added to the documentation.
- Ran unit tests.
- Built and viewed the docs.
| Description | From | Last Updated |
|---|---|---|
|
'MissingAttributesError' imported but unused |
 reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
This isn't the right form for this. You need: __all__ = ['Registry'] |
 chipx86
|
|
|
Could this be NotRegisteredError? |
chipx86
|
|
|
How about AlreadyRegisteredError? DuplicateItemError is far too generic. |
chipx86
|
|
|
I feel like this is too confusing to a consumer of this module. I have no idea what it means … |
chipx86
|
|
|
"Any" isn't really valid for these, I don't think? Let's use object. |
chipx86
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
Col: 50 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 50 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 50 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
'Registry' imported but unused |
reviewbot
|
|
|
This still has the wrong casing for __all__. |
chipx86
|
|
|
Every new file should have a file docstring, so that it's better covered in the module index. It could point … |
chipx86
|
|
|
Two blank lines between these. |
chipx86
|
|
|
These should be localized. While doing that, I'd suggest this formatting: DEFAULT_ERRORS = { 'key': _( 'string', ), ... } … |
chipx86
|
|
|
Needs a docstring. Also, does this need to be a class method? |
chipx86
|
|
|
I don't know if this is the case, but if this isn't present in a subclass that overrides errors, do … |
chipx86
|
|
|
This makes it sound like this requires the items up-front. We should be clear that this is for optionally pre-registering … |
chipx86
|
|
|
Missing a period. |
chipx86
|
|
|
Let's fetch this from _registry only once (we do it again below). |
chipx86
|
|
|
Let's call this unregister_by_attr, so it's explicit. (We may decide we want to unregister by some other thing down the … |
chipx86
|
|
|
Subclasses shouldn't override private methods. I think we need to change the naming of this and populate. What I'd suggest, … |
chipx86
|
|
|
Do we want this to be public? I'm not sure it's needed, since everything else invokes it anyway. |
chipx86
|
|
|
Since _items is a set, this won't provide any sort of stable ordering. Assuming that's okay (probably is), we need … |
chipx86
|
|
|
testing should go last. |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
I don't know that we need to say that they're based on set. That's an implementation detail. This should focus … |
chipx86
|
|
|
"subclassed". |
chipx86
|
|
|
Wrapping is weird. |
chipx86
|
|
|
I don't like having this be stringly typed. Can we make an enum for the error types? |
 david
|
|
|
Reprs typically have <> around them: return '<Item: %s>' % ... |
david
|
|
|
Should be "raising" instead of "returning" |
david
|
|
|
Add another blank line here. |
david
|
|
|
Should use ALREADY_REGISTERED. |
david
|
|
|
This isn't wrapped right. |
david
|
|
|
Too many blank lines here. |
david
|
-
Tool: Pyflakes Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py Tool: PEP8 Style Checker Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py
-
Tool: Pyflakes Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py Tool: PEP8 Style Checker Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py
-
I'll read over this in more detail later (probably when I'm back into town), but I have some general comments up-front:
- I want to avoid putting new things into
djblets.util, since that's how it ends up just growing over time. Certainly there should be no new modules that have submodules in there. We did a big move to split out much of that into their own apps, and this is something I'd much rather see as its owndjblets.registries. - Now that we have codebase docs for Djblets, and are gearing toward making it easier for developers to write code against that and against our extension stuff, I'd very much like to have any new module like this have an accompanying guide or two for writing/using registries in
djblets/docs/djblets/guides/registry/. - The exceptions and terminology used here confuse me, and I think it's too heavy on "items" and "attributes." It makes them sound like dictionaries, but that's not how a consumer of this will think. I'd like the terminology, errors, and docs to be a lot more clear, more obvious.
- Along those lines, error messages should be based on the registry. Talking about "the item is already registered" is too vague and a major step down from what we have. Instead, they should be based on the thing that the registry is dealing with.
'"%s" is already a registered hosting service'is a perfectly fine error, and we should strive to keep that sort of thing.
- I want to avoid putting new things into
-
-
-
-
I feel like this is too confusing to a consumer of this module. I have no idea what it means unless I jump in and look at the implementation. Doesn't a duplicate attribute mean that it's already been registered, and if so, can we not just use the prior exception?
I actually don't understand any of the exceptions in here aside from
DuplicateItemError. -
- Change Summary:
-
Addressed Christian's issues.
- Description:
-
Registries are wrappers around Python's
setclass that provides theadditional constraints: 1. All registered members must have a core set of attributes (defined per registry). 2. The values for these attributes must be unique across all members. Registries also generate convenience methods for retrieving and
unregistering items from the registry for specific attributes. + + A guide on writing registries has been added to the documentation.
- Testing Done:
-
~ Ran unit tests.
~ - Ran unit tests.
+ - Built and viewed the docs.
- Diff:
-
Revision 4 (+705)
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
- Change Summary:
-
Removed the silly metaclass stuff. It was fun but makes it more complicated.
- Description:
-
Registries are wrappers around Python's
setclass that provides theadditional constraints: 1. All registered members must have a core set of attributes (defined per registry). 2. The values for these attributes must be unique across all members. - Registries also generate convenience methods for retrieving and
- unregistering items from the registry for specific attributes. - A guide on writing registries has been added to the documentation.
- Diff:
-
Revision 7 (+540)
-
Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
-
-
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst -
-
I'll give this a more thorough review later (working on student reviews), but can you update the summary to be more self-descriptive? Like, "Add a foundation for implementing object registry APIs" or something?
-
-
-
Every new file should have a file docstring, so that it's better covered in the module index. It could point to the guide as well.
-
-
These should be localized. While doing that, I'd suggest this formatting:
DEFAULT_ERRORS = { 'key': _( 'string', ), ... }That way, it's easier to scan and update, and you get consistent alignment.
-
-
I don't know if this is the case, but if this isn't present in a subclass that overrides errors, do we hit this assert? If so, we need a useful exception for that.
-
This makes it sound like this requires the items up-front. We should be clear that this is for optionally pre-registering items.
I kind of think that this should take an explicit list, though, rather than using
*args. That limits our ability to update it down the road. -
-
-
Let's call this
unregister_by_attr, so it's explicit. (We may decide we want to unregister by some other thing down the road, like maybe an item's class, or something, or a subclass may want a version of this.) -
Subclasses shouldn't override private methods. I think we need to change the naming of this and
populate.What I'd suggest, actually, is having a
get_default_itemsmethod that returns a value, and havepopulatehandle it below. -
-
Since
_itemsis aset, this won't provide any sort of stable ordering. Assuming that's okay (probably is), we need to document that. -
-
-
-
I don't know that we need to say that they're based on
set. That's an implementation detail. This should focus entirely on what they're for, how they're used.Overall, this guide should focus more about walking someone through their purpose and usage. An example and a couple attribute guides are useful, but reading through this, without knowing what registries are for, I wouldn't know what registries are for.
Some high-level examples of their usage (conceptual, with an example for each) would be good. Basically, mini sections on "Here's how you'd do XYZ."
-
-
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py djblets/registries/__init__.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst docs/djblets/coderef/index.rst
-
Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst djblets/registries/__init__.py docs/djblets/coderef/index.rst Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst djblets/registries/__init__.py docs/djblets/coderef/index.rst
-
Tool: PEP8 Style Checker Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst djblets/registries/__init__.py docs/djblets/coderef/index.rst Tool: Pyflakes Processed Files: djblets/registries/errors.py djblets/registries/tests.py djblets/registries/registry.py Ignored Files: docs/djblets/guides/index.rst docs/djblets/guides/registries/index.rst docs/djblets/guides/registries/writing-registries.rst djblets/registries/__init__.py docs/djblets/coderef/index.rst
Tool: Pyflakes Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py Tool: PEP8 Style Checker Processed Files: djblets/util/registry/__init__.py djblets/util/registry/tests.py djblets/util/registry/errors.py djblets/util/registry/registry.py