Opened 17 years ago

Closed 10 years ago

Last modified 10 years ago

#3591 closed New feature (fixed)

add support for custom app_label and verbose_name

Reported by: jkocherhans Owned by: Adrian Holovaty
Component: Core (Other) Version: dev
Severity: Normal Keywords: app-loading
Cc: dirleyrls, domen@…, Mike Scott, robillard.etienne@…, akaihol+django@…, tom@…, v.oostveen@…, dave.lowe@…, research@…, semente@…, gonz@…, ega641@…, mocksoul@…, hv@…, lidaobing@…, remco@…, s.angel@…, drackett@…, gabor@…, seocam@…, django@…, mbeachy@…, camillobruni@…, andy@…, bronger@…, mike@…, the.paper.men@…, mmitar@…, Chris Chambers, aljosa.mohorovic@…, sehmaschine@…, bendavis78@…, Jari Pennanen, mathijs@…, basti@…, JMagnusson, sfllaw@…, mgventura, nils@…, carlos.palol@…, sebastian.goll@…, ionel.mc@…, aav, danols@…, Sander Steffann, pshields@…, flisky, jakub@…, albrecht.andi@…, JonathanBarratt, marc.tamlyn@…, flavio.curella@…, 4glitch@…, luc.saffre@…, zachborboa+django@…, vlastimil@…, django@…, akanouras, unpig, ua_django_bugzilla@…, someuniquename@…, Daniel Samuels Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Once the hot club of france opens and we have more third party apps, the probability of clashing app names will increase. We can fix this by allowing users to define a custom app_label in their settings file. In addition, we can add a verbose_name for apps at the same time. The patch here also correctly assigns and app_label to models in a package as long as you import all of those models in the packages __init__.py This possible breaks quite a few internals if people are using them, and needs to wait until after 0.96 to be considered.

Here's an example:

from django.conf.directives import app

INSTALLED_APPS = (
    'django.contrib.auth', # allow the old syntax
    app('mypkg.auth', 'myauth', 'My cool auth app'), # and the new. (path, app_label, verbose_name)
)

Originally discussed in this thread

Attachments (17)

custom-app-labels.diff (20.6 KB) - added by jkocherhans 17 years ago.
A couple of internal tests are still broken, but admin, manage.py, etc. work
app_labels.diff (28.3 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
A new, more comprehensive (IMHO) patch
app_labels.2.diff (28.1 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
Minor tweaks (tidy-ups) to the last patch.
app_labels.3.diff (28.1 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
Minor tweaks (tidy-ups) to the last patch.
app_labels.4.diff (29.4 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
Updated patch to work cleanly against trunk revision r5171.
app_labels.5.diff (30.9 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
Replaces previous patch which was not created using svn diff - sorry.
app_labels.6.diff (32.1 KB) - added by Vinay Sajip <vinay_sajip@…> 17 years ago.
An updated patch to cater for recent changes in trunk.
app_labels.7.diff (35.1 KB) - added by Vinay Sajip <vinay_sajip@…> 16 years ago.
An updated patch to cater for recent changes in trunk. Applies to r6453.
app_labels.8.diff (35.1 KB) - added by Brian Rosner 16 years ago.
updated to apply cleanly against r6635
app_labels.9.diff (34.0 KB) - added by Vinay Sajip <vinay_sajip@…> 16 years ago.
Updated to apply cleanly against r6920.
app_labels.10.diff (31.7 KB) - added by Vinay Sajip <vinay_sajip@…> 15 years ago.
Updated to apply cleanly against r8965 (post 1.0).
app_labels.11.diff (32.0 KB) - added by Vinay Sajip <vinay_sajip@…> 15 years ago.
Patch should apply cleanly to r9180.
app_labels.12.diff (37.5 KB) - added by George Sakkis 15 years ago.
Patch updated to expose verbose_name in admin; fixed bug in auth; should apply cleanly to r9498
app_labels.13.diff (36.9 KB) - added by Vinay Sajip <vinay_sajip@…> 15 years ago.
Patch updated to apply cleanly to r10759.
app-loading-1.4-alpha.diff (100.9 KB) - added by Ben Davis 12 years ago.
patch against r16630
app-loading.diff (100.8 KB) - added by Jannis Leidel 12 years ago.
correct patch
app-loading.2.diff (101.5 KB) - added by Jannis Leidel 12 years ago.

Download all attachments as: .zip

Change History (174)

Changed 17 years ago by jkocherhans

Attachment: custom-app-labels.diff added

A couple of internal tests are still broken, but admin, manage.py, etc. work

comment:1 Changed 17 years ago by jkocherhans

Needs documentation: set
Needs tests: set
Owner: changed from Adrian Holovaty to jkocherhans
Patch needs improvement: set
Status: newassigned
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 17 years ago by Malcolm Tredinnick

A couple of questions from an initial quick read:

  1. In management.py, why is the import change on line 366 (of the original) necessary? I think there's some subtlety escaping me there (if it's not necessary, it's probably worth removing so that we can tell the real changes from the stylistic ones).
  2. (this question left blank, because it was stupid and I worked out I was missing something obvious.)
  3. Do you really mean you have to import all models in an app's __init__. py (a big change and not ideal, but maybe inavoidable; I realise this is a bit of a tarpit) or just in app_name/models/__init__.py? The stuff in loading.py make me suspect you might mean the latter, but I might be reading it incorrectly.

comment:3 in reply to:  2 Changed 17 years ago by jkocherhans

Replying to mtredinnick:

A couple of questions from an initial quick read:

Hey Malcolm

First of all, thanks for taking a look at this, and second, none of this is meant to be final. This is the "get it working" version that I more or less only put up as a reference for another ticket that this fixes.

  1. In management.py, why is the import change on line 366 (of the original) necessary? I think there's some subtlety escaping me

there (if it's not necessary, it's probably worth removing so that we can tell the real changes from the stylistic ones).

It has an effect. Changing the current import from
from django.db.models import get_models
from django.db.models import get_app, get_models to
would get the right effect.

  1. Do you really mean you have to import all models in an app's __init__. py (a big change and not ideal, but maybe inavoidable; I realise this is a bit of a tarpit) or just in app_name/models/__init__.py? The stuff in loading.py make me suspect you might mean the latter, but I might be reading it incorrectly.

It's the latter. That could probably be worked around as well, but I haven't spent much time thinking about that part of it yet.

comment:4 Changed 17 years ago by Marty Alchin <gulopine@…>

It looks like we have a little bit of end-goal overlap with #4144. The patch over there is just a simple move of a single line, but it relates to how Django determines app_label, so it looks like that might be best handled in this ticket.

Essentially, this patch moves the model_module.__name__.split('.')[-2] into get_app_label, and in fact seems to remove any need for sys.modules at all (unless I'm reading something wrong). If the sys.modules line referenced in #4144 were to be removed as part of this ticket, #4144 would be unnecessary, and I'd be very happy.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.diff added

A new, more comprehensive (IMHO) patch

comment:5 Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I've added an updated patch. This covers changes to make-messages.py and also documentation changes, and is against trunk revision r5146. All tests in runtests.py pass, plus browsing data from apps with a custom app label in admin and databrowse works fine. Here are the relevant changes to my settings.py:

from django.conf import app

# ...

INSTALLED_APPS = (
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.sites',
    'django.contrib.humanize',
    'django.contrib.admin',
    'django.contrib.databrowse',
    'mysite.myapp',
    'mysite.registration',
    app('mysite.orghier', 'oh', 'Organization Hierarchy'),
)


comment:6 Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Marty: Although my latest patch still leaves the line where it is, the model_module is not being used at all. I have removed the line entirely from my working copy - once I get other feedback on the patch, I will post another patch which incorporates the feedback.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.2.diff added

Minor tweaks (tidy-ups) to the last patch.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.3.diff added

Minor tweaks (tidy-ups) to the last patch.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.4.diff added

Updated patch to work cleanly against trunk revision r5171.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.5.diff added

Replaces previous patch which was not created using svn diff - sorry.

Changed 17 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.6.diff added

An updated patch to cater for recent changes in trunk.

comment:7 Changed 16 years ago by anonymous

Cc: simon@… added

comment:8 Changed 16 years ago by Chris H. <chris@…>

I'm seriously +1 on this -- at work we're hoping we'll be using a mix of our custom apps, apps built by other sites in our division and some of the stellar open source apps being built.

I think the modularity/plug-n-customize model of Django's "app" system is one of it's great features!

comment:9 Changed 16 years ago by Adrian Holovaty

I'm recording this idea here: What if INSTALLED_APPS itself was a *class*, rather than a tuple? That would solve a lot of the wacky issues.

comment:10 Changed 16 years ago by Adrian Holovaty

Owner: changed from nobody to Adrian Holovaty
Status: assignednew

comment:11 Changed 16 years ago by Adrian Holovaty

Status: newassigned

comment:12 Changed 16 years ago by robillard.etienne@…

Cc: robillard.etienne@… added

This patch is quite interesting.. ;-)

Would it make sense to use a dictionary and named arguments
for making things more explicit?

In example:

INSTALLED_APPS = (
 app('apps.foobar', {'verbose_name': 'foobar app',
                     'app_label': 'foobar1'}),
)

comment:13 Changed 16 years ago by James Bennett

#2982 was a duplicate.

comment:14 Changed 16 years ago by James Bennett

#3343 needs to be dealt with when a resolution is reached here.

comment:15 Changed 16 years ago by James Bennett

#4470 was a duplicate.

comment:16 Changed 16 years ago by Eli Courtwright <eli@…>

Replying to ubernostrum:

#4470 was a duplicate.

That ticket would have fixed the problem of app_label being required when using the Django ORM outside of an actual Django application. I often write programs at my job which use the Django ORM for non-website programming, but I always have to add the Meta class to the model definition, which is ugly and looks like black magic to my co-workers who aren't familiar with Django internals:

class Something(models.Model):
    class Meta:
        app_label = ""
    
    name        = models.CharField(maxlength=100)
    description = models.TextField()
    quantity    = models.IntegerField()

This ticket doesn't seem to address this problem, whereas ticket #4470 did.

comment:17 Changed 16 years ago by James Bennett

Eli, app_label is required to cleanly split models up into multiple files inside a models module. And I'm doing my best to keep all of the app_label-related stuff here in this ticket so we can get a clean solution that solves all of the associated issues.

comment:18 Changed 16 years ago by Eli Courtwright <eli@…>

That makes sense. I just hope that whatever that clean solution is, it will allow us to define models outside of Django webapps without having to declare the Meta class and manually set app_label.

I'll try to find time this week to take a look at the work on this ticket so far and see how well that solves this problem, and offer any advice/patches I can come up with.

comment:19 Changed 16 years ago by Eli Courtwright <eli@…>

The latest patch doesn't work against the current version of the Django trunk in svn. Nor does it work against r5171, which is the revision that patch 4 is said to work against, nor does it work against r5343, which was the latest revision on the date that the most recent patch was uploaded.

Someone needs to update the patch to work with the current version of Django. I won't have time to do this for at least the next few weeks, and I'm probably the wrong person to do it anyway, since I only use the Django ORM and don't write Django webapps. However, if no one else gets to this by the time I have some more free time, I'll take a stab at it.

comment:20 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

I'm surprised that the last patch doesn't version with the earlier SVN version of Django - before uploading the patch, I had checked that all tests passed. Never mind - I'll try to look at this over the next week or two.

comment:21 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Oops, that should be "... last patch doesn't work ..."

Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.7.diff added

An updated patch to cater for recent changes in trunk. Applies to r6453.

comment:22 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Latest patch (against r6453) passes all tests (tests/runtests.py).

comment:23 Changed 16 years ago by Antti Kaihola

Cc: akaihol+django@… added

Changed 16 years ago by Brian Rosner

Attachment: app_labels.8.diff added

updated to apply cleanly against r6635

comment:24 Changed 16 years ago by Brian Rosner

I tested this patch and updated to apply to current trunk (r6635). All works for me.

However, I do have a question why get_installed_app_paths exists? Why can't it be done in __init__ of Settings when it expands out .* app paths? That way there isn't a need to change all instances of settings.INSTALLED_APPS. Making this completely backward compatible.

comment:25 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Good call - I'd done it the other way because it leaves INSTALLED_APPS untouched, and I wanted the impact of the patch to be clearly visible at least until the overall idea was approved. However, though it's been quite a long time - 8 months - since my first patch, and though there have been no adverse comments and a few comments with the gist "it just works", the devs have not seen fit to pronounce on it - so I'm treading water until I get a pronouncement about it. I just occasionally check to see if trunk changes break the patch, and update the patch accordingly. I'll merge your changes into mine so that my next patch has your changes, too.

comment:26 in reply to:  24 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Replying to brosner:

However, I do have a question why get_installed_app_paths exists? Why can't it be done in __init__ of Settings when it expands out .* app paths? That way there isn't a need to change all instances of settings.INSTALLED_APPS. Making this completely backward compatible.

I had another look at the get_installed_app_paths vs. INSTALLED_APPS issue. With my patch, INSTALLED_APPS can contain either package names (explicit or wild-card) or app instances. However, get_installed_app_paths always returns a set of strings - the package paths of the applications. This is used, as you've seen, in a lot of places. If a user puts an app instance into INSTALLED_APPS, I'm not sure they'd take kindly to having it automatically replaced with the corresponding path string. So, get_installed_app_paths insulates the rest of the code from having to know whether the INSTALLED_APPS entries are path strings or app instances. It seems to me that some kind of encapsulation will be needed - and get_installed_app_paths performs this function. I would like to be able to use INSTALLED_APPS and do away with get_installed_app_paths - but I'm not quite sure how, yet. If you provide a patch which sorts out this issue, I'll happy incorporate it, as I mentioned.

In your testing, did you have any app instances in your INSTALLED_APPS? I'd be interested in seeing what your INSTALLED_APPS looks like. From a quick inspection of your patch, I would expect some tests to fail if INSTALLED_APPS contained any app instances, because in some places where framework code expects a string, it would get an app instance.

comment:27 Changed 16 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:28 Changed 16 years ago by wolfram

is #6080 not a partly shortcut?

Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.9.diff added

Updated to apply cleanly against r6920.

comment:29 in reply to:  28 Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Replying to wolfram:

is #6080 not a partly shortcut?

No, I don't believe #6080 overlaps with this ticket. That ticket is to do with loading apps from eggs; this ticket allows for easy specification of an app_label to disambiguate apps which end in the same name (e.g. 'django.contrib.admin' clashing with 'myapp.mypackage.admin'), and allows verbose names with i18n support for use in the admin (e.g. 'Authentication/Authorization' rather than 'auth').

comment:30 Changed 16 years ago by Thomas Steinacher <tom@…>

Cc: tom@… added

comment:31 Changed 16 years ago by mrts

I believe another feature that would improve admin index page usability a lot belongs conceptually here.

Suppose I have a project that contains 10 applications, each containing several models. Some of the apps are of primay importance, some are less important. Currently, there is no way to impose either ordering or hide app contents. Thus it's hard for users to discern important bits from non-important ones and confusion is guaranteed.

So I propose the following addition to this patch:

  • ordering: app('mypkg.auth', 'myauth', 'My cool auth app', weight=1) # heavier items sink to the bottom in admin index page
  • collapsing: app('mypkg.auth', 'myauth', 'My cool auth app', style={'classes' : ('collapse',)}) # similar behaviour to fieldsets in admin index page

comment:33 Changed 16 years ago by anonymous

Cc: v.oostveen@… added

comment:34 Changed 16 years ago by anonymous

Cc: dave.lowe@… added

comment:35 Changed 16 years ago by anonymous

Cc: research@… added

comment:36 Changed 16 years ago by Johannes Dollinger

I'm strongly -1 on any admin specific arguments to app(). This even includes verbose_name.

IMO the biggest achievement in nfa is that "all admin functionality has been decoupled from the model syntax" (NewformsAdminBranch).

Don't let such a coupling creep in again through app().

comment:37 in reply to:  36 ; Changed 16 years ago by Vinay Sajip <vinay_sajip@…>

Replying to emulbreh:

I'm strongly -1 on any admin specific arguments to app(). This even includes verbose_name.

IMO the biggest achievement in nfa is that "all admin functionality has been decoupled from the model syntax" (NewformsAdminBranch).

Don't let such a coupling creep in again through app().

So what mechanism would you propose for displaying e.g. an internationalized name for an application in the admin?

comment:38 in reply to:  37 Changed 16 years ago by Johannes Dollinger

Replying to Vinay Sajip <vinay_sajip@yahoo.co.uk>:

So what mechanism would you propose for displaying e.g. an internationalized name for an application in the admin?

Something analogous to ModelAdmin classes:

class MyAppAdmin(AppAdmin):
    app = 'mypkg.myapp'
    verbose_name = _('My App')
    description = _('...')
    style = {'classes' : ('collapse',)}
    weight = 1


admin.site.register(MyAppAdmin)

AppAdmin classes could be implicitly created for apps that don't provide one.

I'm not familar with nfa yet (as I'm currently using trunk without admin), so this might be a painfull change.
But it should be doable with a little refactoring (app level permission code then should probably move to AppAdmin as well).

comment:39 Changed 16 years ago by Johannes Dollinger

arg. ignore those _() calls.

comment:40 Changed 16 years ago by Guilherme M. Gondim <semente@…>

Cc: semente@… added

comment:41 Changed 16 years ago by mrts

emulbreh has a good point. The app() directive is really only required for admin, so it does not conceptually belong to settings.py. Perhaps this should be discussed further on django-developers.

comment:42 Changed 16 years ago by Jannis Leidel

At the Pycon sprint there was a rather long discussion about that which later got documented at InstalledAppsRevision

comment:43 Changed 16 years ago by Marty Alchin

I should also throw out there that while Django itself might only use these options in the admin, other apps may make good use of them as well. My dbsettings app, for instance, shows application names on its configuration screen, and I'd rather not reach into the guts of the admin just to get the app's verbose_name. After all, verbose_name isn't being moved out into an admin directive in newforms-admin, is it? I don't see why an app's verbose_name should be treated any differently.

comment:44 Changed 16 years ago by Jannis Leidel

I think we should think of this matter not as a problem with the admin as a general inflexibility of the app loading mechanism.

This results, for example, in the weird behaviour that only apps with a models.py are loaded correctly. I think it's worthwhile to make the app loading happen in a more generic representation, like an App class, just like suggested in InstalledAppsRevision. Such an App could very well include a verbose_name (and other meta data) that would be found by Django and 3rd party apps.

comment:45 Changed 16 years ago by mrts

Admittedly I was wrong about app()'s usefulness only in admin context. path, name and db_prefix parameters listed in InstalledAppsRevision should be generally available. style and weight (order is perhaps a better name) are evidently admin-specific. description lies somewhere in the middle -- it may be useful in other contexts besides admin.

Looks like both app() and something in the lines of AppAdmin are necessesary. Once the presentation layer has been decoupled with AppAdmin, other clever ways for building even more flexible custom admin interfaces utilizing it are free to emerge. Coupling presentation to app() on the other hand is a dead end. So I'm -1 to my own proposal to extend app() with additional keywords, but still +1 for a flexible mechanism for app presentation in the admin index page.

comment:46 Changed 15 years ago by Jannis Leidel

Does the latest patch allow multiple instances of one application?

comment:47 Changed 15 years ago by mrts

The AppAdmin stuff is a separate ticket now, see #7497

comment:48 Changed 15 years ago by Jacob

milestone: 1.0 betapost-1.0

We've passed feature-freeze (mostly), so I'm pushing this post-1.0. :(

comment:49 Changed 15 years ago by anonymous

Cc: gonz@… added

comment:50 Changed 15 years ago by mrts

It should go far beyond what's outlined in InstalledAppsRevision. There's a lot to gain, e.g. no more magic in models autodiscovery, app dependencies etc. Apps should be packaged as eggs and the setuptools config structure setuptools.setup should be extended in Django-specific way. I'll eventually write a spec draft and bring it up in django-developers after 1.0 is out.

comment:51 Changed 15 years ago by anonymous

Cc: ega641@… added

comment:52 Changed 15 years ago by mrts

Another point: think of a way to expose app media (js, css, images) as well.

comment:53 Changed 15 years ago by Vadim Fint <mocksoul@…>

Cc: mocksoul@… added

Changed 15 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.10.diff added

Updated to apply cleanly against r8965 (post 1.0).

comment:54 Changed 15 years ago by Vinay Sajip <vinay_sajip@…>

This latest patch (against r8965) passes all tests (tested with PostgreSQL backend). Please try it out and post your feedback on the mailing lists! Hopefully now that Django 1.0 is out (yay!), the core devs will have more time to review this patch and give some feedback on what's lacking...

comment:55 Changed 15 years ago by Mike Scott

Cc: Mike Scott added

comment:56 Changed 15 years ago by Thomas Güttler

Cc: hv@… added

comment:57 Changed 15 years ago by anonymous

Cc: lidaobing@… added

comment:58 Changed 15 years ago by Remco Wendt

Cc: remco@… added

Changed 15 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.11.diff added

Patch should apply cleanly to r9180.

comment:59 Changed 15 years ago by anonymous

is adrian still around? it's been more than one year since he's last posted here.
no offense intended but maybe it would bring this bug back to life if it weren't assigned?

comment:60 Changed 15 years ago by anonymous

Cc: s.angel@… added

comment:61 Changed 15 years ago by anonymous

Cc: george.sakkis@… added

Changed 15 years ago by George Sakkis

Attachment: app_labels.12.diff added

Patch updated to expose verbose_name in admin; fixed bug in auth; should apply cleanly to r9498

comment:62 Changed 15 years ago by George Sakkis

Updated Vinay's patch so that the admin views expose the app's verbose_name as app_label in the templates; also fixed a bug in django/contrib/auth/management/__init__.py that prevents create_superuser to be called in syncdb.

Applies cleanly to r9498, the latest trunk revision before the the 1.0.2 release. Hope some core-dev takes a look at it and gives feedback before 1.1.

comment:63 Changed 15 years ago by anonymous

Cc: drackett@… added

comment:64 Changed 15 years ago by Gábor Farkas

Cc: gabor@… added
Resolution: fixed
Status: assignedclosed

comment:65 Changed 15 years ago by Jannis Leidel

Resolution: fixed
Status: closedreopened

Please don't close tickets that aren't fixed yet.

comment:66 Changed 15 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:67 Changed 15 years ago by anonymous

Cc: flosch@… added

comment:68 Changed 15 years ago by anonymous

Cc: seocam@… added

Changed 15 years ago by Vinay Sajip <vinay_sajip@…>

Attachment: app_labels.13.diff added

Patch updated to apply cleanly to r10759.

comment:69 Changed 15 years ago by anonymous

Cc: django@… added

comment:70 Changed 14 years ago by mbeachy

Cc: mbeachy@… added

comment:71 Changed 14 years ago by anonymous

Cc: camillobruni@… added

comment:72 Changed 14 years ago by Alex Robbins

Cc: Alex Robbins added

comment:73 Changed 14 years ago by Andy Baker

Cc: andy@… added

comment:74 Changed 14 years ago by anonymous

Cc: domen@… added

comment:75 Changed 14 years ago by Adam Nelson

Triage Stage: AcceptedDesign decision needed

Design Decision still needed according to Alex Gaynor

comment:76 Changed 14 years ago by Torsten Bronger

Cc: bronger@… added

comment:77 Changed 14 years ago by Mike Fogel

Cc: mike@… added

comment:78 Changed 13 years ago by thepapermen

Cc: the.paper.men@… added

comment:79 Changed 13 years ago by Mitar

Cc: mmitar@… added

comment:80 Changed 13 years ago by Russell Keith-Magee

Triage Stage: Design decision neededAccepted

Accepted on the basis that this was a GSoC 2010 project, and the patch is looking reasonably good.

comment:81 Changed 13 years ago by Russell Keith-Magee

Triage Stage: AcceptedFixed on a branch

Sorry - to clarify: The patch that looks good is the GSoC branch, not the patch on this ticket.

comment:82 Changed 13 years ago by Chris Chambers

Cc: Chris Chambers added

comment:83 Changed 13 years ago by aljosa

Cc: aljosa.mohorovic@… added

comment:84 Changed 13 years ago by dirleyrls

Cc: dirleyrls added

comment:85 Changed 13 years ago by anonymous

Cc: sehmaschine@… added

comment:86 Changed 13 years ago by Ben Davis

Cc: bendavis78@… added

comment:87 Changed 13 years ago by anonymous

Cc: Jari Pennanen added

comment:88 Changed 13 years ago by Jari Pennanen

I don't know what is the current suggested syntax for this, but either way it should be defined inside the app, and not during installation of app like in the example of description. E.g. if we allow the app_label be variable that can be defined in INSTALLED_APPS, what happens to all permission checks? Someone installs the auth app as myauth and suddenly all permission checks stops working.

Probably it should be in the __init__.py of app, with something like:

APP_LABEL='myauth'

This way all application developers should define APP_LABEL in the __init__.py and they could use it like namespace, with values like ciantic_myauth. It would also allow backwards compatible way to do this.

comment:89 Changed 13 years ago by Alex Robbins

Cc: Alex Robbins removed

comment:90 Changed 13 years ago by Ben Davis

Does it make sense to allow the overriding of app_label? It seems app_label should only be used internally by django to differentiate between apps. I'm not sure where the need is for overriding app_label in the first place. Wouldn't something like this be enough?

INSTALLED_APPS = (
    'some.foo.app',
    app('another.app', name='My App'),
) 

That way models can be grouped together by name in the admin. One reason I have a need for this is we have a blog app called "zinnia" installed, which has models called "Entries". It uses the django-comments app, which puts all of its models under "comments". Ideally, I should have in the admin an app area called "Blog", with models for "Entries" and "Comments", even though those models live in different apps.

comment:91 Changed 13 years ago by Jannis Leidel

Please have a look at the app-loading branch created during the GSoC 2010 which is scheduled to be included in 1.4.

comment:92 Changed 13 years ago by Jari Pennanen

Okay I think I found something from the branch. There will be App class for each app. This is a good thing, the verbose_name and app_label will be defined inside the app, which is the right thing to do. See the apps.py in GSoC 2010 branch which allowes one to define verbose_name etc. Basically like defining new App class for each app in __init__.py or anywhere for that matter. After which the INSTALLED_APPS is just list of App classes.

Btw it would be helpful if there were a README or something considering the branch, so one would know what and how the branch proposes to implement the feautre. Now I just "luckily" guessed the place for this apps.py.

comment:93 Changed 13 years ago by Jannis Leidel

milestone: 1.4

comment:94 Changed 13 years ago by Julien Phalip

Cross-posting with jezdez :) See #10436 for a related issue.

comment:95 Changed 13 years ago by Mathijs de Bruin

Cc: mathijs@… added

comment:96 Changed 13 years ago by Sebastian Rahlf

Cc: basti@… added

comment:97 Changed 13 years ago by JMagnusson

Cc: JMagnusson added

comment:98 Changed 13 years ago by George Sakkis

Cc: george.sakkis@… removed

comment:99 Changed 13 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:100 Changed 13 years ago by Simon Law

Cc: sfllaw@… added

comment:101 Changed 13 years ago by mgventura

Cc: mgventura added

comment:102 Changed 13 years ago by Nils Fredrik Gjerull

Cc: nils@… added
Easy pickings: unset

comment:103 Changed 13 years ago by Nils Fredrik Gjerull

Easy pickings: set

comment:104 Changed 13 years ago by Jannis Leidel

Easy pickings: unset

Not an easy picking ;)

comment:105 Changed 12 years ago by Ben Davis

Cc: Ben Davis added
UI/UX: unset

Is anyone actively working on this? I'm willing to help bring the soc2010/app-loading branch up to date with trunk, if help is needed.

comment:106 Changed 12 years ago by Ben Davis

UI/UX: set

comment:107 Changed 12 years ago by anonymous

@bendavis78: I am pretty sure your help would be greatly appreciated - considering the amount of attention/CC's for this ticket.

Maybe get on IRC and discuss with one or two of the core devs as to how and whether your work might be merged into trunk as soon as possible - and as to what the bottlenecks and design goals are for this feature. -- irc://irc.freenode.net/django

comment:108 Changed 12 years ago by Jannis Leidel

@bendavis78 Yes, arthurk and me were working on this ticket till a month ago, see https://github.com/jezdez/django/tree/app-loading for the latest state.

comment:109 Changed 12 years ago by Ben Davis

Thanks. For clarity's sake, I'm attaching a patch based on jezdez's app-loading branch that applies cleanly against r16630 (current svn trunk). I also have this on my github fork here: https://github.com/savidworks/django/tree/app-loading.

Also, for the benefit of others interested in this patch, I'll do my best to explain what it does based on what I've gleaned from the source:


The INSTALLED_APPS setting may now contain a reference to an App class. For example:

INSTALLED_APPS = (
    # ...
    'django.contrib.admin',
    'myapp.apps.MyApp',
)

These app classes extend django.apps.App, and may define a number of properties:

from django.apps import App

class MyApp(App):
    class Meta:
        verbose_name = 'My sweet pony app'

Other properties that can be set on Meta are db_prefix and models_path. This will allow you to override existing app definitions in order to prevent potential conflicts.

When the app loader loops through INSTALLED_APPS, it converts any normal modules an App class and loads everything into the apps cache (which was moved from django.models.loading to django.apps.cache). This cache is, among other things, used by the admin for introspection of apps and models.

For further customization, one may also specify how an App class is initialized by providing kwargs in the INSTALLED_APPS setting:

INSTALLED_APPS = (
    # ...
    ('myapp.apps.MyApp', {
        'verbose_name':'Something else',
        'foo':'bar'
    }),
)

In this case, MyApp would be initialized to the equivalent of:

class MyApp(App):
    foo = 'bar'
    class Meta:
        verbose_name = 'Something else'

My only question at this point is regarding the design decision to use an options class (ie, MyApp.Meta). The code suggests that other properties may be set on your custom App class, but how might one use custom properties on an App class? I'm sure there's a reason for not putting verbose_name, db_prefix, and models_path in the class's main properties, I'm just curious what the thinking is on that.

On another note, I was thinking it would also be useful to allow app authors to define how apps should be displayed to the end user. Specifically, I think it would be nice to be able to group and order models in a way that would be more meaningful or helpful to admin users. Perhaps that's one thing that could be done on within a custom App class. Although, that might be a discussion for another time.

Last edited 12 years ago by Ben Davis (previous) (diff)

Changed 12 years ago by Ben Davis

Attachment: app-loading-1.4-alpha.diff added

patch against r16630

Changed 12 years ago by Jannis Leidel

Attachment: app-loading.diff added

correct patch

comment:110 in reply to:  109 Changed 12 years ago by Jannis Leidel

Replying to bendavis78:

Thanks. For clarity's sake, I'm attaching a patch based on jezdez's app-loading branch that applies cleanly against r16630 (current svn trunk). I also have this on my github fork here: https://github.com/savidworks/django/tree/app-loading.

Actually that wasn't really worth the hassle, you merged it wrong (left some code from old commits in admin/options.py). I attached a correct patch.

For further customization, one may also specify how an App class is initialized by providing kwargs in the INSTALLED_APPS setting:

INSTALLED_APPS = (
    # ...
    ('myapp.apps.MyApp', {'verbose_name':'Something else','foo':'bar'}),
)

FWIW, I like the following notation better:

INSTALLED_APPS = (
    # ...
    ('myapp.apps.MyApp', {
        'verbose_name': 'Something else',
        'foo': 'bar',
    }),
)

My only question at this point is regarding the design decision to use an options class (ie, MyApp.Meta). The code suggests that other properties may be set on your custom App class, but how might one use custom properties on an App class? I'm sure there's a reason for not putting verbose_name, db_prefix, and models_path in the class's main properties, I'm just curious what the thinking is on that.

It's simple, Meta is used internally by Django during startup, so it needs to be handled with care, reducing the chance to break a rather fundamental part. In other words, it's internal API when it comes to app loading and is configured depending on the (Python) meta class AppBase. Using it in app-specific implementation details like looking for a specific module in all apps (say for implementing the discovery pattern) is fine though. If you want to override/set a specific option of an app class we need to differ between those internal (via _meta) and the "usual" class or instance attributes.

On another note, I was thinking it would also be useful to allow app authors to define how apps should be displayed to the end user. Specifically, I think it would be nice to be able to group and order models in a way that would be more meaningful or helpful to admin users. Perhaps that's one thing that could be done on within a custom App class. Although, that might be a discussion for another time.

That's certainly a good idea but not really part of this ticket. In other words, this can be added at a later time on top of the new app classes.

comment:111 Changed 12 years ago by Ben Davis

Ah, thanks for catching my mistake in the merge.

It looks like my assumption that an App subclass can implement its own attributes is incorrect with the current patch. Currently the python metaclass AppBase does not propagate the class's attrs to the new_class It only propagates {'__module__': module}:

    def __new__(cls, name, bases, attrs):
        super_new = super(AppBase, cls).__new__
        parents = [b for b in bases if isinstance(b, AppBase)]
        if not parents:
            # If this isn't a subclass of App, don't do anything special.
            return super_new(cls, name, bases, attrs)
        module = attrs.pop('__module__', None)
        new_class = super_new(cls, name, bases, {'__module__': module})
        attr_meta = attrs.pop('Meta', None)
        if not attr_meta:
            meta = getattr(new_class, 'Meta', None)
        else:
            meta = attr_meta
        app_name = attrs.pop('_name', None)
        if app_name is None:
            # Figure out the app_name by looking one level up.
            # For 'django.contrib.sites.app', this would be 'django.contrib.sites'
            app_module = sys.modules[new_class.__module__]
            app_name = app_module.__name__.rsplit('.', 1)[0]
        new_class.add_to_class('_meta', AppOptions(app_name, meta))
        # For easier Meta inheritance
        new_class.add_to_class('Meta', attr_meta)
        return new_class

Thoughts? Should App subclasses not be allowed to have any attributes other than Meta or _name?

comment:112 Changed 12 years ago by Jannis Leidel

No, you misunderstood me, the class attributes of the app classes shouldn't be put into _meta since they are not part of the internal API. This follows the same pattern as the ModelForm and Model classes .

comment:113 Changed 12 years ago by Jannis Leidel

https://github.com/jezdez/django/blob/app-loading/django/apps/base.py#L50-55 shows pretty well how it's supposed to work:

    def __init__(self, **options):
        for key, value in options.iteritems():
            if key in DEFAULT_NAMES:
                setattr(self._meta, key, value)
            else:
                setattr(self, key, value)

Any parameter which is passed during initialization to the app class (e.g. from the settings) ought to be set as an instance attribute, unless it's one of the few internal options. It's a simple matter of easy customization to be able to do that.

Last edited 12 years ago by Jannis Leidel (previous) (diff)

comment:114 Changed 12 years ago by simon@…

Cc: simon@… removed

comment:115 Changed 12 years ago by flosch@…

Cc: flosch@… removed

comment:116 Changed 12 years ago by Ben Davis

Sure, I understand that they shouldn't be passed into _meta. But one would expect any custom attributes on their app class to stay there, and not magically disappear, correct? For example, if I create this:

class MyApp(App):
    foo = "bar"

the foo attribute of the class will magially disappear. This happens because in the python metaclass, new_class is not being given the attributes of the original class.

Changed 12 years ago by Jannis Leidel

Attachment: app-loading.2.diff added

comment:117 Changed 12 years ago by Jannis Leidel

Yeah, apologies, you're absolutely right, I've pushed a fix and attached an updated patch.

comment:118 Changed 12 years ago by Ben Davis

I noticed that in the admin, app titles are not run through .title(), as they were before the patch. It should be:

                        app_dict[app_label] = {
                            'name': apps.find_app(app_label)._meta.verbose_name.title(),
                            'app_url': app_label + '/',
                            'has_module_perms': has_module_perms,
                            'models': [model_dict],

(contrib/admin/sites.py, line 352)

comment:119 Changed 12 years ago by Carlos Palol

Cc: carlos.palol@… added

comment:120 Changed 12 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:121 in reply to:  118 Changed 12 years ago by Ramiro Morales

Replying to bendavis78:

I noticed that in the admin, app titles are not run through .title(), as they were before the patch.

Actually, the application label is, in current admin code, inconsistently run through either the capfirst template filter (or its underlying function in view Python code) or tranformed by the .title() method.

This would be a good chance to get that fixed and IMHO allowing the app author to define the capitalization of the label would be the best approach (now the label can be something more complex that the app module/package name e.g. "TLD Registries Contacts") instead of forcing such a basic treatment like .title() to be always applied (same thing for translators of that label to other languages).

comment:122 Changed 12 years ago by Sebastian Goll

Cc: sebastian.goll@… added

comment:123 Changed 12 years ago by Ionel Cristian Maries

Cc: ionel.mc@… added

comment:124 Changed 12 years ago by Ramiro Morales

In [17389]:

Reverted [17378] until a public API is available for application labels.

This will allow to solve the i18n of such labels in a more holistic way
without the need to introduce temporary, undocumented mechanisms to
translate them.

Refs #3591, #10436

comment:125 Changed 12 years ago by aav

Cc: aav added

comment:126 Changed 12 years ago by Daniel Sokolowski

Cc: danols@… added

comment:127 Changed 12 years ago by Sander Steffann

Cc: Sander Steffann added

comment:128 Changed 12 years ago by pshields@…

Cc: pshields@… added

comment:129 Changed 12 years ago by flisky

Cc: flisky added

comment:130 Changed 12 years ago by Jakub Paczkowski

Cc: jakub@… added

comment:131 Changed 12 years ago by Andi Albrecht

Cc: albrecht.andi@… added

comment:132 Changed 12 years ago by JonathanBarratt

Cc: JonathanBarratt added

comment:133 Changed 12 years ago by Marc Tamlyn

Cc: marc.tamlyn@… added

comment:134 Changed 11 years ago by Anssi Kääriäinen

2b9fb2e6443c04e4415b17083d727bd80047b6e5 fixed a deadlock in app loading. It does not contain tests as it seemed too hard to write one into Django's test suite. So, a heads up: the app-loading refactor should take care not to reintroduce the deadlock condition again. Refs #18251.

comment:135 Changed 11 years ago by Flavio Curella

Cc: flavio.curella@… added

comment:136 Changed 11 years ago by paramburu

Wanted to let you know I've started a discussion on the django developers group that is related to this ticket. I would appreciate your opinions regarding the topic and advice on how to make it go forward.

https://groups.google.com/forum/?fromgroups=#!topic/django-developers/7f0gKen2ces

Thank you in advance for the troubles.

comment:137 Changed 11 years ago by 4glitch@…

Cc: 4glitch@… added

comment:138 Changed 11 years ago by Aymeric Augustin

Triage Stage: Fixed on a branchAccepted

The GSoC branch is out of date.

The latest efforts are in the app-loading branch.

comment:139 Changed 11 years ago by Luc Saffre

Cc: luc.saffre@… added

comment:140 Changed 11 years ago by zachborboa+django@…

Cc: zachborboa+django@… added

comment:141 Changed 11 years ago by Aymeric Augustin

Status: reopenednew

comment:142 Changed 11 years ago by malavon_despana@…

My suggestion: Let the developer declare in the application package's init.py something like:

class Meta: #or whatever the name should be.

verbose_name = _(u"App title for stuff like Admin")
... perhaps more stuff would be convenient here ...

Keepin' seein' the raw name in the admin interface just sucks :(.

comment:143 Changed 11 years ago by Vlastimil Zíma

Cc: vlastimil@… added

comment:144 Changed 11 years ago by ejaury <django@…>

Cc: django@… added

comment:145 Changed 10 years ago by akanouras

Cc: akanouras added

comment:146 Changed 10 years ago by unpig

Cc: unpig added

comment:147 Changed 10 years ago by Matthias Dahl

Cc: ua_django_bugzilla@… added

comment:148 Changed 10 years ago by Russell Keith-Magee

#21018 raised an interesting point about ordering in INSTALLED_APPS that should be tracked as part of this issue.

comment:149 Changed 10 years ago by Tim Graham

Patch needs improvement: set

comment:150 Changed 10 years ago by Evstifeev Roman

Cc: someuniquename@… added

comment:151 Changed 10 years ago by Ben Davis

Cc: Ben Davis removed

comment:152 Changed 10 years ago by Daniel Samuels

Cc: Daniel Samuels added

comment:153 Changed 10 years ago by Aymeric Augustin

UI/UX: unset

comment:154 Changed 10 years ago by Aymeric Augustin

Patch needs improvement: unset

comment:155 Changed 10 years ago by Aymeric Augustin

Has patch: unset

Django now supports a custom verbose_name (in master, which will become 1.7).

There's no patch available for the next steps.

comment:156 Changed 10 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

This ticket has become long and unwieldy.

Since the hardest part of the refactoring is done, I'm going to close it.

One of the two original features is implemented and the other is tracked in #21683.

I'm filing a series of smaller tickets for remaining or related tasks: https://code.djangoproject.com/query?status=!closed&keywords=~app-loading

comment:157 Changed 10 years ago by Aymeric Augustin

Keywords: app-loading added
Note: See TracTickets for help on using tickets.
Back to Top