Opened 10 years ago
Closed 10 years ago
#22719 closed New feature (wontfix)
Proposal: Delayed Form binding (with working example code)
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | Forms | Version: | 1.6 |
Severity: | Normal | Keywords: | bound unbound bind |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hello everybody!
I would like to propose adding a mechanism to "bind" Form-objects in a way other than at instantiation (e.g. Form(request.POST)
).
This idea was inspired by the following assertion by Paul Graham:
"When I see patterns in my programs, I consider it a sign of trouble.
The shape of a program should reflect only the problem it needs to
solve. Any other regularity in the code is a sign, to me at least,
that I'm using abstractions that aren't powerful enough-- often that
I'm generating by hand the expansions of some macro that I need to
write." [ http://www.paulgraham.com/icad.html ]
Let me give a quick example from the Django documentation's "Working with forms":
def contact(request): if request.method == 'POST': form = ContactForm(request.POST) if form.is_valid(): pass else: form = ContactForm() return render(request, 'contact.html', {'form': form,})
[ highlighted: http://pastebin.com/MAKUSrmY ]
This works well when I don't need to supply any keyword arguments. As soon as I want to do that, the code looks like this:
def contact(request): if request.method == 'POST': form = ContactForm(request.POST, prefix='something-%s' % (some_id,), label_suffix=': ') if form.is_valid(): pass else: form = ContactForm(prefix='something-%s' % (some_id,), label_suffix=': ') return render(request, 'contact.html', {'form': form,})
[ highlighted: http://pastebin.com/9FKS9ExU ]
The really bad (IMHO: wrong) thing about this is that the code
prefix='something-%s' % (some_id,), label_suffix=': '
must be duplicated.
I am therefore using this class to bind the Form late:
class FlexiForm(forms.Form): def set_fields(self): for (f_name, attributes) in self._thefields.items(): for (att_name, att_value) in attributes.items(): setattr(self._theinstance.fields[f_name], att_name, att_value) def __init__(self, form, args=None, kwargs=None, fields=None): self._theform = form self._theargs = args or [] self._thekwargs = kwargs or {} self._thefields = fields or {} self.bind() # !: make it possible to use kwargs here def bind(self, *args): self._theinstance = self._theform(*(list(args) + self._theargs), **self._thekwargs) self.set_fields() return self def __getattr__(self, name): return getattr(self._theinstance, name) def __setattr__(self, name, value): inst = self if name not in ['_theform', '_theargs', '_thekwargs', '_thefields', '_theinstance']: inst = self.__theinstance super(FlexiForm, inst).__setattr__(name, value)
[ highlighted: http://pastebin.com/wzY4qWUJ ]
(This is the actual verbatim class from my code. If you spot any errors, please tell!)
Melding this with the above example, we get:
def contact(request): form = FlexiForm(ContactForm, kwargs={'prefix': 'something-%s' % (some_id,), 'label_suffix': ': ',}) if request.method == 'POST': form.bind(request.POST) # substitutes internal ContactForm() with ContactForm(request.POST) if form.is_valid(): pass else: pass # Form already instantiated! return render(request, 'contact.html', { 'form': form, })
[ highlighted: http://pastebin.com/zNA9MzRJ ]
Advantage: keyword (and other) arguments that are vitally important to many of my forms don't need to be duplicated in different locations.
Probability of mistakes/bugs is lowered, maintainability and readability improved.
I am sure there is a more elegant/correct way to do this.
I suspect it should be added to Django's Form class.
I am looking forward to your ...
[x] criticism
[x] reasons why this shouldn't be done
[x] promises that it will be implemented soon
[x] mindless ranting
best regards,
Bernhard
Change History (6)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
That's what came to my mind when I had finished this post ;-)
Unfortunately it has been a while since I wrote this "helper class" so I can't think of any case where your variant would be insufficient OTOH.
This could be the correct solution I was looking for when I wrote this "ugly" class.
comment:3 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Okay, will close this for now. Please reopen if you have any further thoughts on it.
comment:4 by , 10 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
I've pondered over this for a bit longer now.
Conclusion: The duplication still bothers me a lot.
Your example
form_kwargs = {'prefix': 'something-%s' % (some_id,), 'label_suffix': ': '} form = MyForm(request.POST, **form_kwargs) form = MyForm(**form_kwargs)
would, IMO, be better written as
form = MyForm({'prefix': 'something-%s' % (some_id,), 'label_suffix': ': '}) form.bind(request.POST)
because this reduces code length, eliminates redundancy/repetition, is more concise and, to me at least, is clearer. In many cases it would be a wasted effort to introduce & duplicate the variable name 'form_kwargs' three times just to use the same arguments for the same form.
(note: the 'form' object in the first line of my example is equivalent to the 'form' object in the third line of your example.)
I am much in favor of the syntax I proposed.
I still think "binding" of Form objects should happen after instantiation. It's the only thing that differentiates a 'bound' form from an 'unbound' form and shouldn't introduce the necessity of copying all that code.
I would be happy to read your opinion(s) on this :-)
comment:5 by , 10 years ago
From a really quick glance at Form.__init__()
, I cannot see any obvious technical issues that would prevent this from being implemented, but there may be issues in the details. Design decisions are best discussed on the django-developers mailing list through, so I'd recommend to post there.
comment:6 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
form_kwargs = {'prefix': 'something-%s' % (some_id,), 'label_suffix': ': '} form = MyForm(request.POST, **form_kwargs) form = MyForm(**form_kwargs)
can be written as
form_kwargs = {'prefix': 'something-%s' % (some_id,), 'label_suffix': ': '} form = MyForm(request.POST if request.method == 'post' else None, **form_kwargs)
and moved out of the if/else; I really don't see a good reason for bind
I've used this pattern in the past: