Re: [Django] #14580: Cleaning up duplicate code in the admin, add get_formsets_instances
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: contrib.admin | Version: SVN
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by apollo13):
* status: new => closed
* ui_ux: => 0
* resolution: => fixed
* easy: => 0
Comment:
Fixed in r16934 via #8060
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* status: closed => reopened
* resolution: fixed =>
Comment:
The fix for #8060 did touch some of this code, and would require minor
updates to this patch, but unless I'm missing something I don't think it
actually fixed the duplication this ticket was addressing. It added a
`get_inline_instances` method, because the inline instances are now
permissions-dependent rather than static; but the actual code that creates
formsets based on those inline instances is still duplicated with minor
differences several different places in `add_view` and `change_view`. I
noticed that duplication in working on #8060 and I still think it'd be
good to clean it up if we can, so I'd like to leave this ticket open until
we do so.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: timograham@… (added)
* needs_tests: 1 => 0
Comment:
Adding a patch which is purely a code cleanup -- no change in behavior. I
don't see any obvious benefit of making this new method a public API, but
let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by apollo13):
I see one downside in
https://code.djangoproject.com/attachment/ticket/14580/14580.diff#L97 --
''get_formsets'' calls ''get_inline_instances'', but ''_create_fromsets''
could work with a different set of instances, the cleanest way might be to
pass ''inline_instances'' down to ''get_formsets'' (maybe as
''inline_instances=None'' and call ''get_inline_instances'' only if it's
indeed None for backwards compat)
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by apollo13):
Okay, my comment about the backwards-compat was crap, we can't just add
args to the signature of ''get_formsets'' :(. If you look at the docs:
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets
your current patch will break, since the arguments to zip in
https://code.djangoproject.com/attachment/ticket/14580/14580.diff#L97 will
no longer have the same length! Essentially we'd need one method which
returns the inline and the formset together... FWIW, even the current code
is broken in that regard and the docs should drop the inline via
''get_inline_instances'' instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:13>
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by timo):
Thanks for the feedback. When you say "I see one downside" is that in
reference to my comment about not making this a public API?
I haven't taken the time to fully understand the issue you are describing,
but it seems like it may at least be related to #20702. I can take a
closer look at this tomorrow unless you are interested in working up a
solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by apollo13):
I am fine with not making this a public API, #20702 seems to be exactly
what I ment.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:15>
Django
Reporter: apollo13 | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Accepted => Ready for checkin
Comment:
In 402b4a7a20a4f00fce0f01cdc3f5f97967fdb935:
Fixed #14580 -- Cleaned up duplicate code in admin formset handling.
Thanks apollo13 for the report and review.
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: apollo13 | Owner: nobody
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/14580#comment:17>