Opened 3 years ago

Closed 3 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: master
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


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 Changed 3 years ago by Jaap Roes

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 Changed 3 years ago by Jaap Roes

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...


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.


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 3 years ago by Jaap Roes (next)

comment:3 Changed 3 years ago by Jaap Roes

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 Changed 3 years ago by Aymeric Augustin

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 Changed 3 years ago by Jaap Roes

Owner: Jaap Roes deleted
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 Changed 3 years ago by Aymeric Augustin

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