Remove Python 2 compatibility #100
Conversation
|
Hello @timgraham could you check my PR ? Even if coverage decreased, this is still valid, especially with pytest requiring now python >= 3.5 |
|
Take a look at https://code.djangoproject.com/ticket/23919. There are some more cleanups that could be done like replacing |
|
Hello @timgraham any idea how to fix coveralls ? Is it mandatory to validate the PR ? |
|
It looks like you need to find the test that covers those lines on Python 2 but not on Python 3 and adjust it in some way. |
|
Hello @jezdez any chance to review this PR ? Django 3.0 being official, this is blocking because Django no longer includes six Regards |
tox.ini
Outdated
| @@ -21,10 +20,10 @@ usedevelop = true | |||
| commands = make test | |||
| whitelist_externals = make | |||
| deps = | |||
| dj111: Django>=1.11a1,<2.0 | |||
There was a problem hiding this comment.
There's no need to remove a1. IMO that just makes it less obvious how to test with pre-releases in the future.
django_hosts/middleware.py
Outdated
| @@ -80,10 +80,10 @@ def process_response(self, request, response): | |||
| # Find best match, falling back to settings.DEFAULT_HOST | |||
| try: | |||
| host, kwargs = self.get_host(request.get_host()) | |||
| except DisallowedHost: | |||
| except DisallowedHost: # pragma: no cover | |||
There was a problem hiding this comment.
Please write a test that covers this list rather than '# pragma: no cover'.
There was a problem hiding this comment.
I struggle into finding a valid test for this one in Python 3.x, could you help me on this ?
There was a problem hiding this comment.
Sorry, I don't have time to look into it.
There was a problem hiding this comment.
@Djailla just look into tests that override the ALLOWED_HOSTS settings.
Search code for @override_settings(ALLOWED_HOSTS
There was a problem hiding this comment.
and thanks for working on it! we're waiting for this PR to merge too :)
There was a problem hiding this comment.
Why is DisallowedHost exception catching needed on Python 2 but not Python 3? That seems suspicious.
There was a problem hiding this comment.
I did revert my change, the point is the exception is only raised when using Django < 2.0
There was a problem hiding this comment.
I've removed this branch in #114.
|
@timgraham is this and #101 good to merge? |
|
@itsthejoker I'm struggling with the coverage issue. @timgraham any idea ? |
|
@itsthejoker for #101 it requires #100 to be merged first |
|
@Djailla I just checked and the code you reverted was what coveralls was annoyed about. There should be no decrease in coverage now, the tests just need to be re-run. |
|
@Djailla Pinging you to rerun the coveralls check because it should pass now. Maybe you can do it through the github.com UI, otherwise you can make a tiny commit and revert it to trigger the check again. If you don't get back to this soon, I hope you don't mind if I open a duplicate PR myself to get this fixed. |
|
@Jdsleppy all green ! Thanks :) |
django_hosts/resolvers.py
Outdated
| @@ -11,8 +11,7 @@ | |||
| from django.core.exceptions import ImproperlyConfigured | |||
| from django.core.signals import setting_changed | |||
| from django.urls import NoReverseMatch, reverse as reverse_path | |||
| from django.utils import six | |||
| from django.utils.encoding import iri_to_uri, force_text | |||
| from django.utils.encoding import iri_to_uri | |||
| from django.utils.functional import lazy | |||
| from django.utils.lru_cache import lru_cache | |||
There was a problem hiding this comment.
Replace with from functools import lru_cache
|
Could you look at #114? I thought we could rebase this after merging that. |
|
@timgraham I though that #114 was integrating this review, but otherwise, yes no problem for a rebase 👍 |
timgraham
changed the title
Python 2.x will be deprecated early 2020, remove support as Django no longer support either python 2.x