Opened 12 years ago

Closed 11 years ago

#17557 closed Cleanup/optimization (wontfix)

Refactor form CBV to remove some unnecessary complexity

Reported by: Travis Swicegood Owned by: nobody
Component: Generic views Version: 1.4-alpha-1
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While creating a CBV to work with inline formsets, I ran into a few places where copy-n-paste was the only way to adjust the behavior that I was after and keep the same feel of the internal Django CBV. This refactor removes the need to copy-n-paste out of get in order to add additional data by moving the generation of the form context data into get_context_data.

Attachments (1)

improve-cbv-104.patch (9.5 KB ) - added by Travis Swicegood 12 years ago.

Download all attachments as: .zip

Change History (7)

by Travis Swicegood, 12 years ago

Attachment: improve-cbv-104.patch added

comment:1 by Travis Swicegood, 12 years ago

This patch needs a little tweaking. form_valid() should use the minimal get_context_data() call.

comment:2 by Travis Swicegood, 12 years ago

Nope -- nevermind. Jumped the gun on that one. It should use the explicit version here as that version has the cleaned data in it. Also, it was form_invalid that's using get_context_data(form=form).

comment:3 by Travis Swicegood, 12 years ago

From a comment on my commit on GitHub:

How is this simplifying ? Now every subclass has to do the None check (lines 37, 38) instead of just creating the form. Plus, it's backwards incompatible as current implementations assume form_class is never None.

Unfortunately, it does look like get_form_class does document it's behavior. I happen to think this behavior is much more intuitive, but BC concerns should be weighed as well. One potential fix is to revert post back to its original behavior and modify get_context_data to always pass in get_form_class's results when adding the default form to the context. That maintains backwards compatibility, but does end up with a net gain in LOC.

It might help to understand my original thought process to see this code which was my original motivation. I was attempting to add in the inline formset behavior with minimal changes, but after looking at it again this evening I see a few ways I can refactor this code to limit the # of changes. The only problem is that I will be implementing it differently than Django's FormMixin, which feels odd.

Any thoughts?

comment:4 by Tomek Paczkowski, 12 years ago

Triage Stage: UnreviewedDesign decision needed

I've cosulted with Jannis and now marking it as DDN. This isn't important enough to be included in 1.4 IMHO.

comment:5 by Travis Swicegood, 12 years ago

FWIW, I've played with some of this code in the wild and I'm not thrilled by it. Rather than have it linger in DDN, might as well move it over to wontfix and someone can re-open it if they feel strongly about it. I'm still +1 for get_form_class having a form_class=None parameter, but given the BC concerns I'd probably leave it out now.

comment:6 by Aymeric Augustin, 11 years ago

Resolution: wontfix
Status: newclosed

This ticket has stalled. Closing per Travis' last comment.

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