Opened 2 weeks ago

Closed 8 days ago

#35887 closed Cleanup/optimization (fixed)

Improve Examples for StackedInline and TabularInline

Reported by: Alexander Lazarević Owned by: Alexander Lazarević
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alexander Lazarević)

I think the Django admin documentation could be improved by making the examples for StackedInline and TabularInline easier to incorporate. There are some bits and pieces missing sometimes that are not obvious.

Take the partial example to show how TabularInline is meant to be used:

from django.contrib import admin


class BookInline(admin.TabularInline):
    model = Book


class AuthorAdmin(admin.ModelAdmin):
    inlines = [
        BookInline,
    ]

So using this, you'll soon find out that the import for the Book model is missing. Easy enough to add this, but still a nuisance.

from .models import Book

Everything seems to work, but you can't see neither Authors nor Books in the admin, because the registration is missing. So you just quickly type in the code to register the ModelAdmins from your weak memory.

admin.register(Book, BookInline)
admin.register(Author, AuthorAdmin)

You realise the import for the Author model is missing as well. Ok, you just add that.

from .models import Author, Book

Everything looks fine, the server starts without any errors and again no Authors or Books in the admin.

You search through some other examples and see that you used admin.register instead of admin.site.register. After a face-palm you correct it.

admin.site.register(Book, BookInline)
admin.site.register(Author, AuthorAdmin)

Now the server puts out an error "AttributeError: 'BookInline' object has no attribute 'urls'".
You wonder what that's all about and after some time you find an example in the tutorial that only contains the registration for the not inlined ModelAdmin.

admin.site.register(Author, AuthorAdmin)

After that you got it finally working ...

I think providing the import of the models and the registration could prevent such on odyssey.

There are other places as well where some pieces are missing and it's not possible to just copy and paste the examples.

Change History (10)

comment:1 by Alexander Lazarević, 2 weeks ago

Description: modified (diff)

comment:2 by Alexander Lazarević, 2 weeks ago

Has patch: set

comment:3 by Sarah Boyce, 2 weeks ago

Triage Stage: UnreviewedAccepted

comment:4 by Tim Graham, 2 weeks ago

Philosophically, I don't agree that every ModelAdmin example needs to be complete with admin.site.register(). (Why are inlines a special case?) This is reference documentation, not a how to or tutorial that's meant to be copy and pasted. IMO, we should avoid boilerplate that adds additional length. If we do proceed with this, @admin.register might be preferred over admin.site.register().

On another topic the PR addresses, I agree with updating the examples to use relative imports, however, I'd advocate for doing it more comprehensively as in this PR (which was abandoned).

comment:5 by Alexander Lazarević, 2 weeks ago

Philosophically, I don't agree that every ModelAdmin example needs to be complete with admin.site.register().

I personally think the matter is far less philosophical than just practical.
I didn't touch every example, but the ones that start with an import of some sort, because this gives, at least me, the impression, that the example is complete and there are no missing pieces I have to figure out to get it to work.

(Why are inlines a special case?)

I would argue that in this case it should be made clear that Inlines don't need to be registered with Admin but just the ModelAdmin. Neither the reference nor the tutorial mention this explicitly. You say it's obvious? Maybe not for everybody. So I think it's better to show that in an example.

This is reference documentation, not a how to or tutorial that's meant to be copy and pasted.

But I think it should be obvious if it is a complete or a partial example. If we would argue there are only partial examples in the reference documentation, than why include imports at all? They are not crucial to the examples and just add additional length as well?

IMO, we should avoid boilerplate that adds additional length.

I think the benefit of adding one line of admin.site.register() to have a complete example outweighs the benefit of saving that one line by far.

If we do proceed with this, @admin.register might be preferred over. admin.site.register().

Does Django have a preferences? admin.site.register is mentioned first in the docs followed by "there is *also* a decorator for registering your ModelAdmin classes". So I would think the former is the preference?

On another topic the PR addresses, I agree with updating the examples to use relative imports, however, I'd advocate for doing it more comprehensively as in this PR

I focused to improve the examples for the admin and especially the ones for the Inlines. While doing that I also updated them to use relative imports, to make it easier to use them without to have to rename myapp in whatever your app's name is.

I'm aware that there might be other examples, that could use relative imports as well. I could look into this, but would rather keep the scope of this ticket and the PR on the admin doc and its examples.

Last edited 2 weeks ago by Alexander Lazarević (previous) (diff)

comment:6 by Sarah Boyce, 2 weeks ago

There are other examples where imports and registering has been excluded - these are mostly showing a specific ModelAdmin or inline feature (the example has one class definition) - I think these cases it's much more focused to not have the imports etc as it's a clear partial example
However, when the example defines multiple "things" (so a ModelAdmin and an inline for example), these examples are "fuller". If these have imports they should be complete. I also think it's good to be explicit that inlines do not need registering to the admin.

But the PR does include more changes than this so I will re-review with that in mind

comment:7 by Sarah Boyce, 2 weeks ago

Patch needs improvement: set

comment:8 by Alexander Lazarević, 9 days ago

Patch needs improvement: unset

comment:9 by Sarah Boyce, 9 days ago

Triage Stage: AcceptedReady for checkin

comment:10 by Sarah Boyce <42296566+sarahboyce@…>, 8 days ago

Resolution: fixed
Status: assignedclosed

In 8590d05d:

Fixed #35887 -- Added imports and admin.site.register to non-partial admin inline doc examples.

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