Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Don't try to import urlconf unless it's a string. #205

Merged
merged 10 commits into from
Feb 7, 2015
Merged

Don't try to import urlconf unless it's a string. #205

merged 10 commits into from
Feb 7, 2015

Conversation

zenoamaro
Copy link
Contributor

When parsing urls, if request.urlconf is already a module (eg. when using django-subdomains) we shouldn't try to import it.

This checks if urlconf is a string before trying to import it.

@ariovistus
Copy link
Contributor

seems like something that should have a test for it

@zenoamaro
Copy link
Contributor Author

You're perfectly right. Actually, the urlconf argument wasn't tested directly at all.

This adds tests that pass urlconf directly, both using an import string (and an __import__ mock), or passing a mocked module directly.

I added mock (for mocking imports cleanly) and flake8 (for linting) to requirements.py.

@zenoamaro
Copy link
Contributor Author

Okay, after a bit of fiddling with tox and requirements (sorry about that, wasn't really familiar with it) this is passing on all environments.

six is now in the requirements (seems like a good idea, actually) as I've used it to test for string types over py2 and py3. Note that I've also left mock in the requirements.py as nose is also there.

@ariovistus
Copy link
Contributor

six seemed like a good idea to me too, but apparently django already packages it in django.util, so use that if you can.

@zenoamaro
Copy link
Contributor Author

Uh, does GitHub notify on commits or comments only?

@ariovistus
Copy link
Contributor

not sure. thanks for the PR.

ariovistus pushed a commit that referenced this pull request Feb 7, 2015
Don't try to import urlconf unless it's a string.
@ariovistus ariovistus merged commit 3096323 into marcgibbons:master Feb 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants