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 )
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 , 2 weeks ago
Description: | modified (diff) |
---|
comment:2 by , 2 weeks ago
Has patch: | set |
---|
comment:3 by , 2 weeks ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 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 , 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.
comment:6 by , 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 , 2 weeks ago
Patch needs improvement: | set |
---|
comment:8 by , 9 days ago
Patch needs improvement: | unset |
---|
comment:9 by , 9 days ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR https://github.com/django/django/pull/18766