[1.4] Pass "obj" parameter to get_inline_instances

[Yohan Boniface]

Hi,

First of all, congrats to the core team for the first rc of the 1.4 release!

Some great changes have been made on the admin for this release. One of
them is the add of the "get_inline_instances" [1], that gives the
possibility to change the inlines list at runtime.

I've a little suggestion on this. It could be really more powerful if to
this method get passed the "obj" parameter, like for example it's done
for get_fieldsets [2].

For what I can see, this seems to be a very soft change.

The function is called three times in options.py.
One is in get_formsets [3], and get_formsets itself has the "obj" parameter.
Another is in the add_view [4], and here obj could have its default
value, None, as we are in add view and so no object exists.
The last is in the change_view [5], which retrieve the "obj" from its
own parameters.

Do you think this change is enough minor to be added in the 1.4 release
process?

If yes, just tell me if I need to create a ticket and a patch. :)

But maybe, I've missed something! :s

Thanks for all, django guys!

Yohan

[1]
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L348
[2]
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L425
[3]
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L508
[4]
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L936
[5]
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L1034

[Carl Meyer]

Hi Yohan.

On 03/08/2012 07:04 AM, Yohan Boniface wrote:
> Some great changes have been made on the admin for this release. One of
> them is the add of the "get_inline_instances" [1], that gives the
> possibility to change the inlines list at runtime.
>
> I've a little suggestion on this. It could be really more powerful if to
> this method get passed the "obj" parameter, like for example it's done
> for get_fieldsets [2].
>
> For what I can see, this seems to be a very soft change.
>
> The function is called three times in options.py.
> One is in get_formsets [3], and get_formsets itself has the "obj"
> parameter.
> Another is in the add_view [4], and here obj could have its default
> value, None, as we are in add view and so no object exists.
> The last is in the change_view [5], which retrieve the "obj" from its
> own parameters.

I think this is a sensible change.

> Do you think this change is enough minor to be added in the 1.4 release
> process?

Unfortunately, no. This is a new feature, which means it was too late to
add it as soon as the beta was released, let alone after the RC. (After
the RC we're hesitant to even fix bugs unless they are release-blocking
bugs).

This is too bad, because once 1.4 is released there will have to be
consideration of backwards-compatibility for existing
"get_inline_instances" methods that don't accept the "obj" parameter.
This may tip the scales towards considering this not worth doing,
depending what compelling use cases can be presented.

Feel free to file a bug for this and it can be revisited after 1.4 is
released.

Carl

[Yohan Boniface]

Hi Carl,

Thanks for your answer.

I fully understand your point: I'm late in the release process.

In the other hand, can't we consider this as a "bug" for the fact that
this new feature is not consistent with what we can call a "de facto API"?
What I mean is that "request" and "obj" are passed and used almost by:
- get_formset
- get_form
- get_fieldsets
- get_prepopulated_fields
- get_readonly_fields

I know that this is not a documented part of Django. But internal
consistency is, IMHO, one of the pillars of a maintenable framework.

As you've also pointed your-self, adding the "obj" parameter after the
release of "get_inline_instances" will introduce a backward incompatible
change. (For those who will have overwritten the method and so do not
accept the "obj" parameter.)

As an illustration, the "obj" parameter could be used to cleanly
distinguish an add process from a change process (and choose which
inlines to display).

Just to be clear: I'm not saying at all that this could be considered as
a "release blocker". But adding this parameter now will certainly be
much less painful than doing it after the release.

In one hand, we have a release process that do not accept exceptions, in
the other hand we have a quite simple change to do that will be more
painful to do later: I think that it calls a pragmatic judgment.

I've made a ticket with a patch, as you've suggested [1]. And I'll let
you be this pragmatic judge! :)

Will all my respect,

Yohan

[1] https://code.djangoproject.com/ticket/17856