klen / django_markdown

Django markdown support and wysiwig
GNU Lesser General Public License v3.0
391 stars 143 forks source link

django_markdown_static is not a valid tag library #42

Closed deanmalmgren closed 9 years ago

deanmalmgren commented 9 years ago

I was just experimenting with your package for the first time—looks awesome!—and I ran into some trouble when I was trying to use the templatetags to render the javascript editors. Below is a screenshot of the error I was receiving. It looks like django 1.6 is having problems with this line. Any ideas for how to fix?

screen shot 2014-12-10 at 2 53 14 pm

deanmalmgren commented 9 years ago

I think this should fix it, or at least it fixed the problem for me. Is there a reason you were using django.apps.apps.is_installed instead of just checking if the string is in django.conf.settings.INSTALLED_APPS?

deanmalmgren commented 9 years ago

I think this should handle it a bit better, @wldcordeiro. I didn't realize django 1.7 added that apps.is_installed functionality. This patch has a backward compatible fallback for earlier versions of django.

wldcordeiro commented 9 years ago

@deanmalmgren Django 1.7 really changed the way apps are handled and loaded there's plenty of information here. https://docs.djangoproject.com/en/1.7/ref/applications/

Also my original snippet was pretty much ripped right out of Django 1.7's admin app code.

https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_static.py

from django.apps import apps
from django.template import Library

register = Library()

_static = None

@register.simple_tag
def static(path):
    global _static
    if _static is None:
        if apps.is_installed('django.contrib.staticfiles'):
            from django.contrib.staticfiles.templatetags.staticfiles import static as _static
        else:
            from django.templatetags.static import static as _static
    return _static(path)
deanmalmgren commented 9 years ago

@wldcordeiro Wow. Yeah, they did change that a lot. Thanks for the link. I'm not sure if you saw, but I added another patch to this PR that incorporates both the 1.7 functionality as well as the old-school way of doing it for pre-1.7 django versions. Is this good to be merged or would you like to see something else address this issue?

wldcordeiro commented 9 years ago

I like that patch you made, adds backwards compatible support in a pattern that insures Django 1.7 and forward uses the newest method. :+1: @deanmalmgren Only suggestion is to remove line 2 since the import is handled conditionally now.

wldcordeiro commented 9 years ago

Now if @klen could merge this and #41 into the repo it'd be excellent for a 0.8.2 release.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same when pulling 07add9bda6cd709f7c78bce66df7eeb15dbc8527 on deanmalmgren:issue-42 into a32fed5080404e86b18e7055ecc6a11b0becfc55 on klen:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.57%) when pulling 1739f087ccebb2b6e084bbbd94e3928a3d699af6 on deanmalmgren:issue-42 into a32fed5080404e86b18e7055ecc6a11b0becfc55 on klen:master.