Summary

Add registries, which can be used to keep track of unique objects

Review Request #7780 — Created Nov. 25, 2015 and submitted Dec. 11, 2015, 11:20 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Blocks

7785

Reviewers

Groups

djblets

People

Description

Registries are wrappers around Python's set class 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.

Testing Done


Ran unit tests.
Built and viewed the docs.

Issues

Description	From	Last Updated
'MissingAttributesError' imported but unused	reviewbot	Nov. 25, 2015, 5:58 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 25, 2015, 5:58 p.m.
This isn't the right form for this. You need: __all__ = ['Registry']	chipx86	Dec. 7, 2015, 2:42 p.m.
Could this be NotRegisteredError?	chipx86	Nov. 28, 2015, 7:17 p.m.
How about AlreadyRegisteredError? DuplicateItemError is far too generic.	chipx86	Nov. 28, 2015, 7:17 p.m.
I feel like this is too confusing to a consumer of this module. I have no idea what it means …	chipx86	Nov. 28, 2015, 7:17 p.m.
"Any" isn't really valid for these, I don't think? Let's use object.	chipx86	Nov. 28, 2015, 7:17 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 6 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 30, 2015, 6:01 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 6:24 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 6:54 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 9:51 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 11:18 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 11:39 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 11:39 p.m.
'Registry' imported but unused	reviewbot	Nov. 30, 2015, 11:51 p.m.
'Registry' imported but unused	reviewbot	Dec. 1, 2015, 7:59 p.m.
'Registry' imported but unused	reviewbot	Dec. 1, 2015, 7:59 p.m.
'Registry' imported but unused	reviewbot	Dec. 2, 2015, 4:44 p.m.
'Registry' imported but unused	reviewbot	Dec. 2, 2015, 4:44 p.m.
Col: 50 E128 continuation line under-indented for visual indent	reviewbot	Dec. 2, 2015, 4:44 p.m.
Col: 50 E128 continuation line under-indented for visual indent	reviewbot	Dec. 2, 2015, 4:44 p.m.
Col: 50 E128 continuation line under-indented for visual indent	reviewbot	Dec. 2, 2015, 4:44 p.m.
'Registry' imported but unused	reviewbot	Dec. 2, 2015, 4:50 p.m.
This still has the wrong casing for __all__.	chipx86	Dec. 7, 2015, 5:18 p.m.
Every new file should have a file docstring, so that it's better covered in the module index. It could point …	chipx86	Dec. 7, 2015, 10:29 p.m.
Two blank lines between these.	chipx86	Dec. 7, 2015, 5:17 p.m.
These should be localized. While doing that, I'd suggest this formatting: DEFAULT_ERRORS = { 'key': _( 'string', ), ... } …	chipx86	Dec. 7, 2015, 5:16 p.m.
Needs a docstring. Also, does this need to be a class method?	chipx86	Dec. 7, 2015, 5:19 p.m.
I don't know if this is the case, but if this isn't present in a subclass that overrides errors, do …	chipx86	Dec. 7, 2015, 4:54 p.m.
This makes it sound like this requires the items up-front. We should be clear that this is for optionally pre-registering …	chipx86	Dec. 7, 2015, 5:20 p.m.
Missing a period.	chipx86	Dec. 7, 2015, 5:15 p.m.
Let's fetch this from _registry only once (we do it again below).	chipx86	Dec. 7, 2015, 5:20 p.m.
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	Dec. 7, 2015, 5:21 p.m.
Subclasses shouldn't override private methods. I think we need to change the naming of this and populate. What I'd suggest, …	chipx86	Dec. 7, 2015, 5:12 p.m.
Do we want this to be public? I'm not sure it's needed, since everything else invokes it anyway.	chipx86	Dec. 7, 2015, 5:15 p.m.
Since _items is a set, this won't provide any sort of stable ordering. Assuming that's okay (probably is), we need …	chipx86	Dec. 7, 2015, 5:21 p.m.
testing should go last.	chipx86	Dec. 7, 2015, 5:21 p.m.
Missing period.	chipx86	Dec. 7, 2015, 5:21 p.m.
Two blank lines.	chipx86	Dec. 7, 2015, 10:30 p.m.
I don't know that we need to say that they're based on set. That's an implementation detail. This should focus …	chipx86	Dec. 7, 2015, 11 p.m.
"subclassed".	chipx86	Dec. 7, 2015, 11 p.m.
Wrapping is weird.	chipx86	Dec. 7, 2015, 11 p.m.
I don't like having this be stringly typed. Can we make an enum for the error types?	david	Dec. 11, 2015, 4:16 p.m.
Reprs typically have <> around them: return '<Item: %s>' % ...	david	Dec. 11, 2015, 4:16 p.m.
Should be "raising" instead of "returning"	david	Dec. 11, 2015, 4:16 p.m.
Add another blank line here.	david	Dec. 11, 2015, 6:43 p.m.
Should use ALREADY_REGISTERED.	david	Dec. 11, 2015, 6:43 p.m.
This isn't wrapped right.	david	Dec. 11, 2015, 10:01 p.m.
Too many blank lines here.	david	Dec. 11, 2015, 10:01 p.m.

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

djblets/util/registry/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'MissingAttributesError' imported but unused
```
djblets/util/registry/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

PEP8

Diff:

Revision 2 (+535)

Show changes

	djblets/util/registry/__init__.py
	djblets/util/registry/errors.py
	djblets/util/registry/registry.py
	djblets/util/registry/tests.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

Change Summary:

Add tests, contains, and fix unregister by attributes

Diff:

Revision 3 (+614)

Show changes

	djblets/util/registry/__init__.py
	djblets/util/registry/errors.py
	djblets/util/registry/registry.py
	djblets/util/registry/tests.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 own djblets.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.

djblets/util/registry/__init__.py (Diff revision 3)

The issue has been resolved. Show all issues

This isn't the right form for this. You need:

__all__ = ['Registry']

djblets/util/registry/errors.py (Diff revision 3)

The issue has been resolved. Show all issues

Could this be NotRegisteredError?

brennie Nov. 28, 2015, 5:42 p.m.

Thats not really what this is.

This is the case for when you have a registry for e.g. the id attribute, but try to look up something by name, etc.

djblets/util/registry/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
How about AlreadyRegisteredError? DuplicateItemError is far too generic.
```

djblets/util/registry/errors.py (Diff revision 3)

The issue has been resolved. Show all issues

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.

djblets/util/registry/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Any" isn't really valid for these, I don't think? Let's use object.
```

Change Summary:

Addressed Christian's issues.

Description:

		Registries are wrappers around Python's `set` class 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.

		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)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```
djblets/registries/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

PEP8

Diff:

Revision 5 (+706)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Change Summary:

typo

Diff:

Revision 6 (+706)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 'Registry' imported but unused
```

Change Summary:

Removed the silly metaclass stuff. It was fun but makes it more complicated.

Description:

		Registries are wrappers around Python's `set` class 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.

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

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 8 (+546)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 9 (+546)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 10 (+550)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 10)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 11 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 12 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 13 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 14 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

Diff:

Revision 15 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```
djblets/registries/registry.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 50
 E128 continuation line under-indented for visual indent
```
djblets/registries/registry.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 50
 E128 continuation line under-indented for visual indent
```
djblets/registries/registry.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 50
 E128 continuation line under-indented for visual indent
```

Diff:

Revision 16 (+552)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/__init__.py (Diff revision 16)
The issue has been dropped. Show all issues
```
 'Registry' imported but unused
```

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?

Summary:

-	Add registries
+	Add registries, which can be used to keep track of unique objects

djblets/registries/__init__.py (Diff revision 16)
The issue has been resolved. Show all issues
```
This still has the wrong casing for __all__.
```

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

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.

djblets/registries/registry.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Two blank lines between these.
```

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

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.

djblets/registries/registry.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Needs a docstring.
Also, does this need to be a class method?
```

djblets/registries/registry.py (Diff revision 16)

The issue has been dropped. Show all issues

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.

brennie Dec. 7, 2015, 4:54 p.m.

It looks up the error in self.errors followed by self._default_errors, so we should never hit this for a valid error name.

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

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.

djblets/registries/registry.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing a period.
```
djblets/registries/registry.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Let's fetch this from _registry only once (we do it again below).
```

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

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

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

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_items method that returns a value, and have populate handle it below.

djblets/registries/registry.py (Diff revision 16)

The issue has been dropped. Show all issues

Do we want this to be public? I'm not sure it's needed, since everything else invokes it anyway.

brennie Dec. 7, 2015, 5:15 p.m.

Theres not harm in having a method to ensure its populated.

djblets/registries/registry.py (Diff revision 16)

The issue has been resolved. Show all issues

Since _items is a set, this won't provide any sort of stable ordering. Assuming that's okay (probably is), we need to document that.

djblets/registries/tests.py (Diff revision 16)
The issue has been resolved. Show all issues
```
testing should go last.
```
djblets/registries/tests.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing period.
```
docs/djblets/guides/registries/writing-registries.rst (Diff revision 16)
The issue has been resolved. Show all issues
```
Two blank lines.
```

docs/djblets/guides/registries/writing-registries.rst (Diff revision 16)

The issue has been resolved. Show all issues

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

docs/djblets/guides/registries/writing-registries.rst (Diff revision 16)
The issue has been resolved. Show all issues
```
"subclassed".
```
docs/djblets/guides/registries/writing-registries.rst (Diff revision 16)
The issue has been resolved. Show all issues
```
Wrapping is weird.
```

Change Summary:

Address Christian's issues.

Diff:

Revision 17 (+718)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

Diff:

Revision 18 (+723)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/registry.py (Diff revision 18)

The issue has been resolved. Show all issues

I don't like having this be stringly typed. Can we make an enum for the error types?

djblets/registries/tests.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Reprs typically have <> around them:
return '<Item: %s>' % ...
```
docs/djblets/guides/registries/writing-registries.rst (Diff revision 18)
The issue has been resolved. Show all issues
```
Should be "raising" instead of "returning"
```

Change Summary:

Address David's issues.

Diff:

Revision 19 (+726)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

djblets/registries/registry.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Add another blank line here.
```
djblets/registries/registry.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Should use ALREADY_REGISTERED.
```

Change Summary:

Address David's issues

Diff:

Revision 20 (+727)

Show changes

	djblets/registries/__init__.py
	djblets/registries/errors.py
	djblets/registries/registry.py
	djblets/registries/tests.py
	docs/djblets/coderef/index.rst
	docs/djblets/guides/index.rst
	docs/djblets/guides/registries/index.rst
	docs/djblets/guides/registries/writing-registries.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

Ship it!

Just a couple trivial issues in the docs and then this looks good to me.

docs/djblets/guides/registries/writing-registries.rst (Diff revision 20)
The issue has been resolved. Show all issues
```
This isn't wrapped right.
```
docs/djblets/guides/registries/writing-registries.rst (Diff revision 20)
The issue has been resolved. Show all issues
```
Too many blank lines here.
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (5f282bc)