Code

Opened 2 years ago

Closed 23 months ago

#18751 closed Cleanup/optimization (fixed)

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

Reported by: charettes Owned by: nobody
Component: Forms Version: master
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 charettes 2 years ago.
ticket-18751.1.diff (1.4 KB) - added by charettes 2 years ago.
ticket-18751.2.diff (2.0 KB) - added by charettes 2 years ago.
10711-2.diff (1.6 KB) - added by charettes 2 years ago.
claudep's patch for #10711 which happens to fix this issue

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by charettes

comment:1 Changed 2 years ago by charettes

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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)

Changed 2 years ago by charettes

comment:2 Changed 2 years ago by charettes

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

comment:3 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

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?

Changed 2 years ago by charettes

comment:4 Changed 2 years ago by charettes

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

Last edited 2 years ago by charettes (previous) (diff)

Changed 2 years ago by charettes

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

comment:5 Changed 2 years ago by charettes

  • Triage Stage changed from Accepted to Ready 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 Changed 23 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.