ANN: newforms-admin syntax change for inlines
Joseph Kocherhans
functionality, you want to read this.
I'm planning on breaking the current syntax for defining inlines in
the very near future, but I figured I'd warn people and ask for
comments first. Currently it's something like this:
class MyModelAdmin(ModelAdmin):
inlines = [
StackedInline(MyModel, extra=2)
]
In order to fix a few newforms-admin bugs, the inline class will take
the parent model and the AdminSite object as arguments to its
constructor. This means that the 'inlines' attribute will need to be a
list of classes rather than a list of instances now. The most obvious
way to do it looks like this:
class MyInline(StackedInline):
model = MyModel
extra = 2
class MyModelAdmin(ModelAdmin):
inlines = [
MyInline
]
I'm still thinking about a more convenient syntax for the simple case,
probably a couple of factory functions that act like the old
StackedInline and TabularInline classes.
I'm thinking something like:
class MyModelAdmin(ModelAdmin):
inlines = [
stacked(MyModel, extra=2)
]
where stacked is a function that makes a new class that inherits from
StackedInline, applies the kwargs to that class and returns it. Other
ideas are welcome.
Joseph
Russell Keith-Magee
>
> If you use the newforms-admin branch and the edit inline
> functionality, you want to read this.
>
> I'm planning on breaking the current syntax for defining inlines in
> the very near future, but I figured I'd warn people and ask for
> comments first. Currently it's something like this:
I don't have any partiular objections to the new syntax, but can you
give a quick rundown of why these changes are necessary? The only
reason that comes to my Sunday-addled brain at the moment is that it
would make 'inline within an inline' forms easier to write. Is this
the reason, or is there another that I'm missing?
Also - how does this affet the proposal to merge the 'inlines' clause
into the 'fields' clause (and following your other proposal - the
'fieldsets' clause)? Again, I can't see any reason why it shouldn't be
possible, but I wanted to make sure we weren't making a rod for our
own backs.
Yours,
Russ Magee %-)
Honza Král
>
> On 9/9/07, Joseph Kocherhans <jkoch...@gmail.com> wrote:
> >
> > If you use the newforms-admin branch and the edit inline
> > functionality, you want to read this.
> >
> > I'm planning on breaking the current syntax for defining inlines in
> > the very near future, but I figured I'd warn people and ask for
> > comments first. Currently it's something like this:
>
> I don't have any partiular objections to the new syntax, but can you
> give a quick rundown of why these changes are necessary? The only
> reason that comes to my Sunday-addled brain at the moment is that it
> would make 'inline within an inline' forms easier to write. Is this
> the reason, or is there another that I'm missing?
I would guess one of the reasons is http://code.djangoproject.com/ticket/4491
> Also - how does this affet the proposal to merge the 'inlines' clause
> into the 'fields' clause (and following your other proposal - the
> 'fieldsets' clause)? Again, I can't see any reason why it shouldn't be
> possible, but I wanted to make sure we weren't making a rod for our
> own backs.
I would like for the inlines to be separate, the reason is that in our
system, we have quite a few generic stuff that can bind to any model
and I would like to be able to register these generic models to any
model's admin without the model's knowledge. Like this I could keep
the basic model options simple and uphold the DRY principle.
For example (I know it can be confusing, like my explanations ;) ):
class Article( models.Model ):
...
some_fields_go_here
...
class ArticleOptions( admin.ModelAdmin ):
...
some_basic_options
...
----
class CommentOptions( models.Model ):
enabled = models.Fireld()
moderators = models.Field()
...
for model, model_options in admin.site._registry.items():
if i_like_model( model ):
model_options.inlines.append( CommentOptionsInline )
this would probably still be possible using the fields/fieldsets
property, but this seems cleaner...
>
> Yours,
> Russ Magee %-)
>
> >
>
--
Honza Král
E-Mail: Honza...@gmail.com
ICQ#: 107471613
Phone: +420 606 678585
Joseph Kocherhans
> >
> > If you use the newforms-admin branch and the edit inline
> > functionality, you want to read this.
> >
> > I'm planning on breaking the current syntax for defining inlines in
> > the very near future, but I figured I'd warn people and ask for
> > comments first. Currently it's something like this:
>
> I don't have any partiular objections to the new syntax, but can you
> give a quick rundown of why these changes are necessary? The only
> reason that comes to my Sunday-addled brain at the moment is that it
> would make 'inline within an inline' forms easier to write. Is this
> the reason, or is there another that I'm missing?
Would you believe me if I just said it makes the internals *much* more
consistent and easier to understand? It's part of a refactoring that
changes the wrapper classes related to inlines use a lot more of the
existing admin form wrapper constructs. A desire to refactor things
and http://code.djangoproject.com/ticket/4491 is where this started.
I suppose I could make the syntax backwards-compatible by naming the
factory functions the same as the existing classes, but I think it
would make things more confusing.
> Also - how does this affet the proposal to merge the 'inlines' clause
> into the 'fields' clause (and following your other proposal - the
> 'fieldsets' clause)? Again, I can't see any reason why it shouldn't be
> possible, but I wanted to make sure we weren't making a rod for our
> own backs.
If we decide to put inline definitions into fields or fieldsets, this
change won't make it any more difficult. I'd rather cross that bridge
a little later though. I have thought about both the syntax and the
implementation for that and I have a few ideas, but I don't have the
time to dedicate to it just yet.
Joseph
Malcolm Tredinnick
[...]
> I suppose I could make the syntax backwards-compatible by naming the
> factory functions the same as the existing classes, but I think it
> would make things more confusing.
I think you're more across the subtleties of this feature a lot more
than I am, Joseph, so I don't really have a strong opinion about it.
However, this quoted bit caught my attention.
My gut reaction is that this would be trying to over-finesse things a
bit. I mean, code related to Admin configuration already has to change;
it's already a once-off disruption. Providing some backwards-compat
wrapper here isn't really going to ease that burden enough to be worth
the forwards maintenance hassle, so just go with the one (right) way to
do things and avoid confusion.
Malcolm
--
Quantum mechanics: the dreams stuff is made of.
http://www.pointy-stick.com/blog/
Joseph Kocherhans
> If you use the newforms-admin branch and the edit inline
> functionality, you want to read this.
>
> I'm planning on breaking the current syntax for defining inlines in
> the very near future, but I figured I'd warn people and ask for
> comments first.
I've checked this in as r6072. No factory functions yet. I'll probably
hold off on those until the sprint, or start another thread.
http://code.djangoproject.com/changeset/6072
Joseph
pm13
keys. By default there is one SQL query for each row - result is not
cached. So I used this code in formfield_for_dbfield:
if isinstance(db_field, models.ForeignKey):
kwargs['cache_choices'] = True
The argument cache_choices was used in ModelChoiceField for caching
and I had only one query for one foreign key in inline.
I think that there was call of function "formset_for_model" in each
request (ModelAdmin.get_inline_formsets -> inline_formset ->
formset_for_model). But now this method is called from
ModelAdmin.__init__ (ModelAdmin.__init__ -> InlineModelAdmin.__init__
-> inline_formset -> formset_for_model).
So if I have two users with different query, I can't cache it. But it
is not possible to have 100 SQL queries (if I have 100 rows). I see
only two solution - use the previous variant (and call
formset_for_model in each request) or somehow intelligently bind form,
its fields and formset (but it seems to be complex). But I don't know
django well - maybe there are other possibilities. What do you think?
I created example of the first solution:
http://code.djangoproject.com/ticket/5372
On 9 Zář, 21:24, "Joseph Kocherhans" <jkocherh...@gmail.com> wrote:
Joseph Kocherhans
>
> I think that there was call of function "formset_for_model" in each
> request (ModelAdmin.get_inline_formsets -> inline_formset ->
> formset_for_model). But now this method is called from
> ModelAdmin.__init__ (ModelAdmin.__init__ -> InlineModelAdmin.__init__
> -> inline_formset -> formset_for_model).
Why does that cause a problem for you?
> So if I have two users with different query, I can't cache it. But it
> is not possible to have 100 SQL queries (if I have 100 rows). I see
> only two solution - use the previous variant (and call
> formset_for_model in each request) or somehow intelligently bind form,
> its fields and formset (but it seems to be complex). But I don't know
> django well - maybe there are other possibilities. What do you think?
I can't interpret what you mean by this. Could you try explaining it
another way?
> I created example of the first solution:
> http://code.djangoproject.com/ticket/5372
The patch you attached in that ticket looks like it will solve your
problem, and in fact, you can just subclass TabularInline or
StackedInline, override the formfield_for_dbfield on you subclass, and
add the property from your patch. No patch to Django needed. What am I
missing?
Could you explain why this snippet wouldn't work?
CachingStackedInline(StackedInline):
def formfield_for_dbfield(self, db_field, **kwargs):
if isinstance(db_field, models.ForeignKey):
kwargs['cache_choices'] = True
return super(InlineModelAdmin,
self).formfield_for_dbfield(db_field, **kwargs)
def _formset_class(self):
return forms.inline_formset(self.parent_model, self.model,
fk_name=self.fk_name, formfield_callback=self.formfield_for_dbfield,
extra=self.extra)
formset_class = property(_formset_class)
Joseph
pm13
> >http://code.djangoproject.com/ticket/5372
>
> The patch you attached in that ticket looks like it will solve your
> problem, and in fact, you can just subclass TabularInline or
> StackedInline, override the formfield_for_dbfield on you subclass, and
> add the property from your patch. No patch to Django needed. What am I
> missing?
>
> Could you explain why this snippet wouldn't work?
>
> CachingStackedInline(StackedInline):
> def formfield_for_dbfield(self, db_field, **kwargs):
> if isinstance(db_field, models.ForeignKey):
> kwargs['cache_choices'] = True
> return super(InlineModelAdmin,
> self).formfield_for_dbfield(db_field, **kwargs)
>
> def _formset_class(self):
> return forms.inline_formset(self.parent_model, self.model,
> fk_name=self.fk_name, formfield_callback=self.formfield_for_dbfield,
> extra=self.extra)
> formset_class = property(_formset_class)
>
> Joseph
I am sorry. You are missing nothing - I don't use python for long time
and I haven't realize yet it is possible to override attribute by
property (now I see that it is logical.)
But I think anyway that there should be some kind of caching by
default. It isn't only my problem - it may be quite usual to use
inlines with more rows and with foreign keys. (I will try to give
better description and example to the ticket - I hope tomorrow).
Petr
Joseph Kocherhans
> and I haven't realize yet it is possible to override attribute by
> property (now I see that it is logical.)
It's ok. I just felt I was misunderstanding your problem. :)
> But I think anyway that there should be some kind of caching by
> default. It isn't only my problem - it may be quite usual to use
> inlines with more rows and with foreign keys. (I will try to give
> better description and example to the ticket - I hope tomorrow).
I disagree about caching by default, but it definitely should be
possible to turn caching on. I don't think there is anywhere in Django
where caching is on by default.
Also, take a look at [6080]. I needed to add a couple of hooks to
InlineModelAdmin that would probably be useful for you.
http://code.djangoproject.com/changeset/6080
Joseph
pm13
> > But I think anyway that there should be some kind of caching by
> > default. It isn't only my problem - it may be quite usual to use
> > inlines with more rows and with foreign keys. (I will try to give
> > better description and example to the ticket - I hope tomorrow).
>
> I disagree about caching by default, but it definitely should be
> possible to turn caching on. I don't think there is anywhere in Django
> where caching is on by default.
Probably caching between requests is not on by default anywhere - but
my ticket is about caching in one request. (I don't want one query for
two request.)
I try to create example models - http://code.djangoproject.com/ticket/5372#comment:2.
Yes, it is possible (after [6080]) to turn caching on - but I hope
that you will see that people will expect to have caching on by
default in this situation.
>
> Also, take a look at [6080]. I needed to add a couple of hooks to
> InlineModelAdmin that would probably be useful for you.
>
> http://code.djangoproject.com/changeset/6080
I like it very much. But I am afraid that there is a little problem.
Inlines without fields and fieldsets doesn't work after this changeset
- see http://code.djangoproject.com/ticket/5383.
And thank you for your great work on newforms-admin!
Petr
Joost Cassee
On 9 sep 2007, 14:36, "Honza Král" <honza.k...@gmail.com> wrote an
example newforms-admin snippet that would programmatically add an
inline:
> class CommentOptions( models.Model ):
> enabled = models.Fireld()
> moderators = models.Field()
> ...
>
> for model, model_options in admin.site._registry.items():
> if i_like_model( model ):
> model_options.inlines.append( CommentOptionsInline )
This does not work with the current revision of the newforms-admin
branch. I think it ends up adding the inlines to the
django.contrib.sites.models.SiteAdmin class, but I don't really
understand the code here.
Is there a way to extend the inlines list after the class has been
defined?
Regards,
Joost