Summary

Add a form for working with key/value stores instead of models.

Review Request #7853 — Created Jan. 7, 2016 and submitted Jan. 30, 2016, 10:55 a.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

Forms generally are bound to models, or are completely custom and devoid
of any real default logic beyond rendering and validation. We have some
forms in Djblets that interface with models or other objects, treating
them like key/value stores using set and get methods. These are
all based on SiteSettingsForm, which is built for SiteConfiguration,
and isn't always the best model to use.

This change introduces a new form, KeyValueForm, which works as a base
for all these other forms, SiteSettingsForm included. It handles the
loading of data from an object, the saving of form data back out to
the object, and load/save blacklists for fields (a feature used by
these other forms). It also handles easily marking some fields as
disabled, another feature used by these other forms.

It's built to be usable with any type of key/value storage backend,
by providing several functions that can be overridden to handle the
getting and setting of keys and their values and the creation/saving
of instances.

Testing Done

Unit tests pass in both Djblets and Review Board.

Manually tested configuring extensions and saving the various site
settings forms.

Issues

Description	From	Last Updated
Instead of "list or tuple", how about "iterable"?	david	Jan. 12, 2016, 3:29 p.m.
The final clause in this sentence is superfluous (you already said "the values from that instance").	david	Jan. 12, 2016, 3:30 p.m.
status, not statuses	david	Jan. 12, 2016, 3:30 p.m.
Use six.iteritems?	david	Jan. 12, 2016, 3:31 p.m.
Typo: cange	david	Jan. 12, 2016, 3:31 p.m.

Tool: Pyflakes
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py

In your description, "void of" should be "devoid of"

djblets/forms/forms/key_value_form.py (Diff revision 1)

Show all issues

Instead of "list or tuple", how about "iterable"?

chipx86 Jan. 12, 2016, 3:32 p.m.

I'm actually just going to go with "list", as in a list of things, not necessarily the type. I think it's clear enough, and is what the Django docs do anyway.

djblets/forms/forms/key_value_form.py (Diff revision 1)

Show all issues

The final clause in this sentence is superfluous (you already said "the values from that instance").

djblets/forms/forms/key_value_form.py (Diff revision 1)
Show all issues
```
status, not statuses
```

djblets/forms/forms/key_value_form.py (Diff revision 1)

Show all issues

Use six.iteritems?

chipx86 Jan. 12, 2016, 3:36 p.m.

six.iterkeys. For what it's worth, it's legal to iterate over a dictionary. You'll just get the keys. Switching anyway to make it more clear.

david Jan. 12, 2016, 3:46 p.m.

What I mean is that you're fetching both the key and the value, so you could just do:

for field_name, field in six.iteritems(self.fields):

chipx86 Jan. 12, 2016, 7:26 p.m.
```
Oh. Right. Silly me.
```

djblets/forms/forms/key_value_form.py (Diff revision 1)
Show all issues
```
Typo: cange
```

Change Summary:


Fixed wording in docs and change description.
Switched to using six.iterkeys for iterating through the fields.

Description:

~		Forms generally are bound to models, or are completely custom and void
	~	Forms generally are bound to models, or are completely custom and devoid
		of any real default logic beyond rendering and validation. We have some
		forms in Djblets that interface with models or other objects, treating
		them like key/value stores using `set` and `get` methods. These are
		all based on `SiteSettingsForm`, which is built for `SiteConfiguration`,
		and isn't always the best model to use.

		This change introduces a new form, `KeyValueForm`, which works as a base
		for all these other forms, `SiteSettingsForm` included. It handles the
		loading of data from an object, the saving of form data back out to
		the object, and load/save blacklists for fields (a feature used by
		these other forms). It also handles easily marking some fields as
		disabled, another feature used by these other forms.

		It's built to be usable with any type of key/value storage backend,
		by providing several functions that can be overridden to handle the
		getting and setting of keys and their values and the creation/saving
		of instances.

Diff:

Revision 2 (+553 -60)

Show changes

	djblets/extensions/forms.py
	djblets/forms/forms/__init__.py
	djblets/forms/forms/key_value_form.py
	djblets/forms/tests/__init__.py
	djblets/forms/tests/test_key_value_form.py
	djblets/siteconfig/forms.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py

Change Summary:

Switched the for loop to six.iteritems, simplifying the code.

Diff:

Revision 3 (+551 -60)

Show changes

	djblets/extensions/forms.py
	djblets/forms/forms/__init__.py
	djblets/forms/forms/key_value_form.py
	djblets/forms/tests/__init__.py
	djblets/forms/tests/test_key_value_form.py
	djblets/siteconfig/forms.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/forms.py
    djblets/forms/tests/test_key_value_form.py
    djblets/forms/forms/key_value_form.py
    djblets/siteconfig/forms.py
    djblets/forms/forms/__init__.py

Ignored Files:
    djblets/forms/tests/__init__.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (dc7e111)

my first review

chipx86 March 7, 2016, 4:51 p.m.

This isn't a demo server. Please don't post tests here. You can use http://demo.reviewboard.org/ for this instead.

Ship it!

```
ok
```