Opened 11 years ago

Closed 11 years ago

#20549 closed Cleanup/optimization (wontfix)

Refactor to take advantage of get_app_paths()

Reported by: Aymeric Augustin Owned by:
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

b55624a0263e31be80fbe7bb51e47ccd253e7a5d added a get_app_paths() method in the app cache to simplify fixture loading.

This method could be reused to simplify other code that discovers files at conventional locations inside apps (templates, translations, etc.)

Change History (6)

comment:1 by Jaap Roes, 11 years ago

Has patch: set
Owner: changed from nobody to Jaap Roes
Status: newassigned
Triage Stage: UnreviewedAccepted

I've identified two locations that were fairly easy to convert:

comment:2 by Jaap Roes, 11 years ago

I've also looked at the get_javascript_catalog view as it also traverses app directories, but I'm confused by its behavior. It takes a packages param and then re-declares it in a strange conditional list comprehension. It also doesn't reverse the packages list, is it's behavior different from trans_real? I'm not sure if get_app_paths can work there...

django/views/i18n.py

I also thought that contrib.staticfiles would be a good candidate (the docstring for get_app_paths mentions it), but it doesn't seem feasible without changing the current AppDirectoriesFinder/AppStaticStorage API.

django/contrib/staticfiles/finders.py
django/contrib/staticfiles/storage.py

Is changing the first two code paths an acceptable fix for this ticket? Do you know of more places where get_app_paths should be used?

One last note: get_app_paths actually returns paths to models.pyc, that was not what I expected. In loaddata's fixture_dirs there's some calls to os.path.abspath, os.path.realpath and os.path.dirname, maybe this should all be done in get_app_paths itself?

Version 0, edited 11 years ago by Jaap Roes (next)

comment:3 by Jaap Roes, 11 years ago

Mmmm, running the tests again and AppResolutionOrderI18NTests still fails, thought I fixed it but apparently not. Not sure what causes the failure though...

comment:4 by Aymeric Augustin, 11 years ago

I didn't work on this ticket because I noticed that most places that could take advantage of get_app_paths actually depend on the list of applications in INSTALLED_APPS, and not on the list of applications in the app cache.

And it's currently possible for the app cache to contain applications that aren't in INSTALLED_APPS. Switching to get_app_paths could change the results of the lookups by adding more applications to the search list.

Until app loading is in a better shape, I'm not sure we can move on with this.

comment:5 by Jaap Roes, 11 years ago

Owner: Jaap Roes removed
Status: assignednew
Triage Stage: AcceptedSomeday/Maybe

Cool, was thinking this would be an easy one ;) Not going to put more time into this then.

comment:6 by Aymeric Augustin, 11 years ago

Resolution: wontfix
Status: newclosed

get_app_paths() was deprecated during the app-loading refactor. This isn't relevant any more.

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