Code

Opened 6 years ago

Closed 6 years ago

#7852 closed (invalid)

Cross-application model inheritence broken after newforms-admin merge

Reported by: jbalonso Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: model inheritance
Cc: jalonso@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I have an application that needs to register models for the admin interface in one app that also has a base class for models in two other apps. After the newforms-admin merge, it is impossible to import the base class into another app, since it needs to be referenced by an app.model name string instead of the class reference itself.

My recommendation is to create a models.base_class() function that accepts an app.model string and returns a dynamic class that can be derived. If such a beast already exists, it needs to be documented.

To clarify...

Old:

from django.db import models
from remote.models import Foo

class Bar(Foo):
    # etc.

Proposed:

from django.db import models

class Bar(models.base_class('remote.Bar')):
    # etc.

Attachments (0)

Change History (4)

comment:1 Changed 6 years ago by jbalonso

  • Cc jalonso@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 follow-up: Changed 6 years ago by mtredinnick

That proposed API is terrible. We are programming Python here and subclassing has a well-established syntax. It looks like something about the new admin code's importing needs to be fixed. Certainly "breaking" Python isn't the solution here.

comment:3 in reply to: ↑ 2 Changed 6 years ago by jbalonso

Replying to mtredinnick:

That proposed API is terrible. We are programming Python here and subclassing has a well-established syntax. It looks like something about the new admin code's importing needs to be fixed. Certainly "breaking" Python isn't the solution here.

Very well. Then I suggest the API for registering models for the admin interface either be taken out of models.py files entirely or wrapped in a class so the registration code isn't run on every import.

I think any solution to this that will require a backwards-incompatible API change. As such, I think this should be resolved by the 1.0 release. Being a relatively rookie ticket submitter, I'm not going to set the milestone myself, though.

comment:4 Changed 6 years ago by brosner

  • Resolution set to invalid
  • Status changed from new to closed

There already is a convention for registering models with the admin interface that does not involve models.py. Originally models.py was the only decently safe route before an admin.py module was introduced for applications. All ModelAdmin classes should be placed in an admin.py file in the app. Registration with the default AdminSite instance should be done there only. If you have a need for your own AdminSite then do so at a project-level admin.py and do registration there. The admin.autodiscover() function will take care of doing the imports on the admin.py modules of the INSTALLED_APPS. See the documentation for more information. I am assuming you are seeing AlreadyRegistered exceptions, if so keep this closed, otherwise re-open with specific details.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.