Summary

Add a registry for configuration forms

Review Request #7827 — Created Dec. 24, 2015 and submitted Jan. 28, 2016, 2:06 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Blocks

7844, 7828

Reviewers

Groups

djblets

People

Description

Review Board's "account pages", which are Djblets ConfigPages with
dynamically determined forms have been ported to Djblets. The behaviour
for dynamically configuring a form is encapsulated in the
DynamicConfigPageMixin and the ConfigPageRegistry. Config pages
that are to be dynamic should inherit from the DynamicConfigPageMixin
and are to be managed by a single registry; multiple registries can
manage multiple sets of configuration pages, however.

Testing Done


Ran unit tests.
Used this with an upcoming change for Review Board.

Issues

Description	From	Last Updated
All of our other errors in this form use a capital letter after the ":" (since it's often another exception's …	chipx86	Jan. 4, 2016, 6:06 p.m.
You can do: CONFIG_PAGE_REGISTRY_DEFAULT_ERRORS = dict({ FORM_ALREADY_REGISTERED: ..., ..., }, **DEFAULT_ERRORS)	chipx86	Jan. 4, 2016, 6:04 p.m.
It'd be nice if this went into more detail as to where/how you'd use this.	chipx86	Jan. 4, 2016, 6:16 p.m.
Blank line between these.	chipx86	Jan. 4, 2016, 6:06 p.m.
Trailing whitespace.	david	Jan. 4, 2016, 6 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 4, 2016, 6 p.m.
This needs the full class path.	chipx86	Jan. 4, 2016, 6:06 p.m.
Do we want to guard against registered_forms possibly being page_class.form_classes?	chipx86	Jan. 4, 2016, 6:15 p.m.
Swap these.	chipx86	Jan. 4, 2016, 6:14 p.m.
Typo on "djblets"	chipx86	Jan. 4, 2016, 6:14 p.m.
No blank line.	chipx86	Jan. 4, 2016, 6:14 p.m.
This should probably go into a little more detail on what the result is.	chipx86	Jan. 4, 2016, 6:13 p.m.
Can't we just return if self._populated is False?	chipx86	Jan. 4, 2016, 6:13 p.m.
Seems this would be a lot better as: for item in self._items.copy(): self.unregister(item)	chipx86	Jan. 4, 2016, 6:11 p.m.
We should probably only call iter once.	david	Jan. 4, 2016, 5:59 p.m.
Maybe a little more detail: """... Objects using this mixin are built to have a dynamic list of forms, which …	chipx86	Jan. 25, 2016, 4:45 p.m.
Two blank lines.	chipx86	Jan. 25, 2016, 4:37 p.m.
I'd like to see more here about what this is for and how it's used.	chipx86	Jan. 25, 2016, 4:45 p.m.
No need for this blank line.	chipx86	Jan. 25, 2016, 4:37 p.m.
These should go up above the classes. Maybe put a comment above each saying what class they're for.	chipx86	Jan. 25, 2016, 4:37 p.m.
No need for the blank line.	chipx86	Jan. 25, 2016, 4:37 p.m.
Maybe write a bit here and with the remove function a to how this should be invoked. I assume callers …	chipx86	Jan. 25, 2016, 4:50 p.m.
"it" -> "the registry as".	chipx86	Jan. 25, 2016, 4:38 p.m.

djblets/configforms/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```

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

The issue has been dropped. Show all issues

We should probably only call iter once.

brennie Dec. 29, 2015, 2:28 p.m.

If we call iter only once, we will be modifying the object while iterating through it.

next(iter(s)) for a set s is just getting an arbitrary element in the set so that we can remove it.

brennie Dec. 29, 2015, 2:30 p.m.

The only other way I can see to do this is to make a list of self._items but that seems pretty inefficient.

djblets/configforms/registry.py (Diff revision 1)

The issue has been resolved. Show all issues

All of our other errors in this form use a capital letter after the ":" (since it's often another exception's error appearing after), so we should be consistent there.

We also generally have the form of Could not register the form %(item)r: This form is already registered. Same sort of thing below.

djblets/configforms/registry.py (Diff revision 1)

The issue has been dropped. Show all issues

You can do:

CONFIG_PAGE_REGISTRY_DEFAULT_ERRORS = dict({
   FORM_ALREADY_REGISTERED: ...,
   ...,
}, **DEFAULT_ERRORS)

brennie Jan. 4, 2016, 6:05 p.m.
```
These are not equivalent.
```

djblets/configforms/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
It'd be nice if this went into more detail as to where/how you'd use this.
```
djblets/configforms/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/configforms/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This needs the full class path.
```

djblets/configforms/registry.py (Diff revision 1)

The issue has been resolved. Show all issues

Do we want to guard against registered_forms possibly being page_class.form_classes?

brennie Jan. 4, 2016, 6:16 p.m.

Yes because we will be modifying the list while iterating through it.

djblets/configforms/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Swap these.
```
djblets/configforms/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Typo on "djblets"
```
djblets/configforms/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.
```
djblets/registries/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably go into a little more detail on what the result is.
```
djblets/registries/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can't we just return if self._populated is False?
```

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

The issue has been resolved. Show all issues

Seems this would be a lot better as:

for item in self._items.copy():
    self.unregister(item)

Tool: Pyflakes
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst

djblets/configforms/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Change Summary:

Addressed David and Christian's issues.

Diff:

Revision 2 (+400)

Show changes

	djblets/configforms/mixins.py
	djblets/configforms/registry.py
	djblets/configforms/tests.py
	djblets/registries/registry.py
	docs/djblets/coderef/index.rst

Tool: Pyflakes
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst

Diff:

Revision 3 (+404)

Show changes

	djblets/configforms/mixins.py
	djblets/configforms/registry.py
	djblets/configforms/tests.py
	djblets/registries/registry.py
	docs/djblets/coderef/index.rst

Tool: Pyflakes
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst

djblets/configforms/mixins.py (Diff revision 3)

The issue has been resolved. Show all issues

Maybe a little more detail:

"""...

Objects using this mixin are built to have a dynamic list of forms,
which can be controlled by callers. New forms can be registered on
them, and existing forms unregistered. Through this, it's possible
to extend a configuration page, allowing each form to load, validate,
and save without the page hard-coding support for each form.

Forms utilizing this mixin are expected to <whatever they're expected
to do if using this mixin, such as setting a registry, and what that
should be like, and how they should render it, etc.>
"""

We should always assume in the docs that the user has no idea what this thing is for, why they'd use it, how they'd use it, or any details whatsoever about the implementation.

djblets/configforms/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Two blank lines.
```

djblets/configforms/registry.py (Diff revision 3)

The issue has been dropped. Show all issues

I'd like to see more here about what this is for and how it's used.

brennie Jan. 25, 2016, 4:39 p.m.

This really isn't intended to be used externally. Only the ConfigPageRegistry should be.

djblets/configforms/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No need for this blank line.
```

djblets/configforms/registry.py (Diff revision 3)

The issue has been resolved. Show all issues

These should go up above the classes. Maybe put a comment above each saying what class they're for.

djblets/configforms/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No need for the blank line.
```

djblets/configforms/registry.py (Diff revision 3)

The issue has been resolved. Show all issues

Maybe write a bit here and with the remove function a to how this should be invoked. I assume callers should only call this through the mixin, and not directly?

brennie Jan. 25, 2016, 4:49 p.m.

The methods really only exist on the mixin because they existed on RB's config page stuff.

djblets/registries/registry.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"it" -> "the registry as".
```

Change Summary:

Address Christian's issues.

Diff:

Revision 4 (+422)

Show changes

	djblets/configforms/mixins.py
	djblets/configforms/registry.py
	djblets/configforms/tests.py
	djblets/registries/registry.py
	docs/djblets/coderef/index.rst

Tool: Pyflakes
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst



Tool: PEP8 Style Checker
Processed Files:
    djblets/configforms/registry.py
    djblets/registries/registry.py
    djblets/configforms/mixins.py
    djblets/configforms/tests.py

Ignored Files:
    docs/djblets/coderef/index.rst

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (67d3a0b)