Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7899 closed (fixed)

Some small changes to FormSets

Reported by: Peter of the Norse <RahmCoff@…> Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This diff file contains two minor changes to FormSets.

  1. _max_form_count is renamed to max_form_count to make it constant with extra, etc.
  1. BaseInlineFormset now accepts prefix so that you can have more than one on a age.

Attachments (2)

formset.diff (3.9 KB) - added by Peter of the Norse <RahmCoff@…> 7 years ago.
little diff
7899_tests.1.diff (724 bytes) - added by brosner 7 years ago.
failing tests

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by Peter of the Norse <RahmCoff@…>

little diff

comment:1 Changed 7 years ago by brosner

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

Where is _max_form_count leaking to that is causing the inconsistence? The lack of prefix on BaseInlineFormset is a good catch. Ideally it was reported separately as we prefer individual tickets that describe a single problem.

comment:2 Changed 7 years ago by brosner

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 7 years ago by Peter of the Norse <RahmCoff@…>

My reason for wanting _max_form_count changed was “I don’t use the factory functions and so I set it myself.” But as I sat down to justify it, I realized that there is more that should be changed. _max_form_count is the archetypical class value. It’s only used by __init__ and it doesn’t change between instances. In fact, it should be removed from ManagementForm entirely. The “MAX_FORM” value does nothing. And if people need the value in the template they can always use {{set.max_form_count}}.

I’ve updated the diff file, but I’ll wait to post it until a decision is made.

comment:4 Changed 7 years ago by brosner

No, there is a reason for MAX_FORMS being in the ManagementForm. Javascript code that wants to dynamically create new forms on the fly will need it honor a maximum set by the application logic. I understand the reasoning you have and it really should just be renamed to max_num.

comment:5 Changed 7 years ago by brosner

Ugh, after reading the rest of your comment, I think I might step back and agree with you.

comment:6 Changed 7 years ago by brosner

(In [8058]) Made the semi-private _max_form_count live on the public API of formsets by renaming it to max_num. This also removes the ManagementForm use of MAX_COUNT since that usage should just be referenced to the formset's max_num property. Refs #7899. Thanks Peter of the Norse for straightening me out.

comment:7 Changed 7 years ago by brosner

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

(In [8060]) Added the ability to customize the prefix value on an inline formset. Fixes #7899. AThanks for the tip Peter of the Norse.

Changed 7 years ago by brosner

failing tests

comment:8 Changed 7 years ago by brosner

I posted the tests to the wrong window. Ignore ;)

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