Skip to content

Add dynamic loader class support #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from

Conversation

robgolding
Copy link

Also drops support for older versions of Python/Django, and introduces support for newer ones. Those changes may or may not be acceptable here, but I wanted to open this for an example of the dynamic loader class support, which we're using to load the stats file via staticfiles_storage.open().

Copy link
Collaborator

@owais owais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks good. Added some comments.

try:
urlpatterns.append(url(r'^admin/', include(admin.site.urls)))
except ImproperlyConfigured:
urlpatterns.append(url(r'^admin/', admin.site.urls))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for Django backward compatibility? I'd suggest to import django, check the version and have an if -else clause based on the version to make it obvious for the reader.



class WebpackLoader(object):
_assets = {}

def __init__(self, name='DEFAULT'):
def __init__(self, config, name='DEFAULT'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be remove the default param as we always explicitly pass the config now.

django111: django>=1.11.0,<2.0
django22: django>=2.2.0,<2.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also support 2.1 and 2.0 explicitly.

@owais
Copy link
Collaborator

owais commented Jan 17, 2020

I suppose this is not needed anymore now that we've merged another PR that implemented custom load classes?

@owais owais closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants