Opened 4 years ago

Closed 2 years ago

#17557 closed Cleanup/optimization (wontfix)

Refactor form CBV to remove some unnecessary complexity

Reported by: tswicegood 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 tswicegood 4 years ago.

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by tswicegood

comment:1 Changed 4 years ago by tswicegood

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 4 years ago by tswicegood

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 Changed 4 years ago by tswicegood

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 Changed 4 years ago by oinopion

  • Triage Stage changed from Unreviewed to Design 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 Changed 4 years ago by tswicegood

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 Changed 2 years ago by aaugustin

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

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

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