Opened 3 years ago

Closed 3 years ago

#20549 closed Cleanup/optimization (wontfix)

Refactor to take advantage of get_app_paths()

Reported by: aaugustin 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 jaap3

  • Has patch set
  • Owner changed from nobody to jaap3
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 3 years ago by jaap3

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 jaap3 (next)

comment:3 Changed 3 years ago by jaap3

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 aaugustin

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 jaap3

  • Owner jaap3 deleted
  • Status changed from assigned to new
  • Triage Stage changed from Accepted to Someday/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 aaugustin

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

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