Opened 12 years ago

Closed 12 years ago

#18751 closed Cleanup/optimization (fixed)

`BaseFormSet._should_delete_form` should use `form.cleaned_data`

Reported by: Simon Charette Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords: formset
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Now that #5524 is fixed the BaseFormSet._should_delete_form method could be cleaned-up a bit. Original method comment:

# The way we lookup the value of the deletion field here takes
# more code than we'd like, but the form's cleaned_data will
# not exist if the form is invalid.

Patch coming up.

Attachments (4)

ticket-18751.0.diff (981 bytes ) - added by Simon Charette 12 years ago.
ticket-18751.1.diff (1.4 KB ) - added by Simon Charette 12 years ago.
ticket-18751.2.diff (2.0 KB ) - added by Simon Charette 12 years ago.
10711-2.diff (1.6 KB ) - added by Simon Charette 12 years ago.
claudep's patch for #10711 which happens to fix this issue

Download all attachments as: .zip

Change History (10)

by Simon Charette, 12 years ago

Attachment: ticket-18751.0.diff added

comment:1 by Simon Charette, 12 years ago

Has patch: set

All tests pass on python 2.7.3rc1 sqlite3. I'm wondering if we shouldn't add and accessor for Form.cleaned_data that would call trigger validation under the hood, the same way Form.errors does. We could still raise an AttributeError if form.is_bound() == False:

def _get_cleaned_data(self):
    if not self.is_bound():
        raise AttributeError("'%s' object has no attribute 'cleaned_data'" % self.__class__.__name__)
    self.errors # Accessing `errors` since it knows whether or not it should call `full_clean`
    return self._cleaned_data

def _set_cleaned_data(self, value):
    self._cleaned_data = cleaned_data

cleaned_data = property(_get_cleaned_data, _set_cleaned_data)

by Simon Charette, 12 years ago

Attachment: ticket-18751.1.diff added

comment:2 by Simon Charette, 12 years ago

Added a new patch that fixes a bogus testcase which was saving a formset before making sure it was valid.

comment:3 by Claude Paroz, 12 years ago

Triage Stage: UnreviewedAccepted

Interestingly, I have uploaded a similar patch to #10711 (https://code.djangoproject.com/attachment/ticket/10711/10711-2.diff). I will close it in favour of this one. Maybe there is something to merge between both patches?

by Simon Charette, 12 years ago

Attachment: ticket-18751.2.diff added

comment:4 by Simon Charette, 12 years ago

this patch looks better thank mines since you don't have to change the test suite and you reuse the deleted_forms code path.

Version 0, edited 12 years ago by Simon Charette (next)

by Simon Charette, 12 years ago

Attachment: 10711-2.diff added

claudep's patch for #10711 which happens to fix this issue

comment:5 by Simon Charette, 12 years ago

Triage Stage: AcceptedReady for checkin

claudep, I'm marking the patch you've written for #10711 as RFC since it passes all tests (python 2.7.3rc1 sqlite3) and is the definitive way to clean up _should_delete_form with backward compatibility in mind. My patches we're incomplete since they introduced an obscure raises of AttributeError when attempting to save a not-yet-validated formset (which is a foolish thing to do anyway), see ticket-18751.2.diff testsuite changes.

comment:6 by Claude Paroz <claude@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [fb3d916c201d5882098bbcc94f60765412ba7aff]:

Fixed #18751 -- Cleaned up BaseFormSet._should_delete_form

We can do that now that cleaned_data is guaranteed to be
present. Related to [121fd109].
Thanks Simon Charette for his work on the ticket.

Note: See TracTickets for help on using tickets.
Back to Top