ervandew / eclim

Expose eclipse features inside of vim.
http://eclim.org
GNU General Public License v3.0
1.04k stars 127 forks source link

Django: improved template loading (via Python API) #274

Closed blueyed closed 10 years ago

blueyed commented 10 years ago

eclim#python#django#util#GetTemplateDirs does not find template directories of apps.

It just looks at the output from "manage.py diffsettings" and uses TEMPLATE_DIRS from there.

I wonder if it is a good idea to just call django.template.loader via Python to find it?

from django.template import loader
t = loader.find_template( "app/model_list.html" )[0]
t.origin.name 

(might raise TemplateDoesNotExist)

btw: when using unipath (from unipath import Path), you might end up with something like the following in TEMPLATE_DIRS: Path('/project/foo/templates') (the workaround here is to use str() for the setting in TEMPLATE_DIRS). (unipath is used in the project template, I am not sure, if it's that useful).

ervandew commented 10 years ago

I've been making a lot of changes as I migrate eclim over to pydev, so I'll take a look at this once I get the rest of that branch ready to be merged into master. I like the idea of letting django find the template, so i'll probably go that route. I'll also be sure to test with unipath.

Thanks for the suggestions!

blueyed commented 10 years ago

Ok.

I already have a few fixes to the django part of eclim (GetProjectPath, GetSettings, ..) - I'll try to provide them somehow, so you can re-use / integrate it. (Of course, GetSettings should look at Django's settings module then, too)

ervandew commented 10 years ago

FYI, I made some updates to the FindTemplate function on my pydev branch so that it will find templates from installed apps now. I didn't use the django loader code you provided since as your commit notes, it requires django 1.7 which hasn't been released yet, so that's a non-starter since a lot of users (myself included) probably won't be on 1.7 until potentially long after it is released.

https://github.com/ervandew/eclim/commit/bea1d820f62c87f2ca6d511b3845e26772ad5dfd#diff-915a441659a352f30071367e37de169aR184

blueyed commented 10 years ago

Yes, Django 1.7 is required (or the django-debug-toolbar, which appears to add the same information).

I would use this as a first attempt, and when it fails, use the other method. That makes it more reliable altogether.

It would need a good abstraction, and then could be used to lookup settings (with a fallback to parsing the output from diffsetting), too.

Do you consider the pydev branch usable already? I have noticed that you keep rebasing it, which might make it difficult to track.

ervandew commented 10 years ago

This should be resolved now that pydev branch has been merged into master.

blueyed commented 10 years ago

Hmm, not really.

I think the issue should be about preferably "using Django's Python API" instead of manage.py diffsettings etc.

I have my old local changes stashed, and will see if I can up with something based on the pydev-rewrite in the next days/weeks.

ervandew commented 10 years ago

I don't like the idea of using apis that aren't available to all django users. Falling back to non-django apis for older django versions leads to the fallback code not being as well tested.