Update to suport Django 1.10 #56
Conversation
| from django.db.utils import DEFAULT_DB_ALIAS | ||
| from django.http import HttpResponse | ||
| from django.utils.six import StringIO | ||
| from smuggler import settings | ||
| import django |
There was a problem hiding this comment.
Put the import statement on top. I would also suggest a line space between from smuggler... and from django... * stuff.
That is a recommendation from PEP 8:
Imports should be grouped in the following order:
- standard library imports
- related third party imports
- local application/library specific imports
- You should put a blank line between each group of imports.
The same applies to the changes at file tests/test_app/tests/test_dump.py.
| @@ -30,15 +33,21 @@ def serialize_to_response(app_labels=None, exclude=None, response=None, | |||
| error_stream = StringIO() | |||
| dumpdata = DumpData() | |||
| dumpdata.style = no_style() | |||
| dumpdata.execute(*app_labels, **{ | |||
| args = { | |||
There was a problem hiding this comment.
I would name it kwargs instead args. The same applies at the similar change below.
| extra_url_kwargs = {'prefix': 'admin'} \ | ||
| if StrictVersion(django.get_version()) < StrictVersion('1.7')\ | ||
| else\ | ||
| {} |
There was a problem hiding this comment.
I like to avoid the use of \. But in this particular case, I recommend use inlines only when it can fit in one line. My recommendation is to use the following code:
if StrictVersion(django.get_version()) < StrictVersion('1.7'):
extra_url_kwargs = {'prefix': 'admin'}
else:
extra_url_kwargs = {}
There was a problem hiding this comment.
By the way, I would rename the variable extra_url_kwargs to login_url_kwargs.
There was a problem hiding this comment.
Wait, we drop support to Django < 1.7 so we don't need it actually.
Removed code required only on Django < 1.7.
|
Ok great suggestions, as i said first time doing this PR thing so thank you for the tips. About coverage thing, i don't know exactly how to fix it. If condition is evaluated under an specific environment? |
1 similar comment
|
@semente coverage remains at 100% and travis now also tests Django 1.10. Will you prepare a release? |
|
@jaap3 perfect! I will release it today. |
Django 1.10 uses natural key for sites, so pk needs to be removed from SITE_DUMP
Using call_command instead of execute to avoid 'no_color' KeyError
prefix param in url not supported on Django1.10
I usually not do PR, most likely is wrong, but maybe someone can improve it.