Skip to content
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

Add Django2.0 compatibility #65

Closed
wants to merge 1 commit into from
Closed

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Oct 23, 2017

And drop django versions lower than 1.11

@ticosax ticosax force-pushed the django-2.0 branch 2 times, most recently from 52b4ca8 to 7eeef25 Compare October 23, 2017 08:30
.travis.yml Outdated
- python: "pypy3"
env: DJANGO='Django<1.11'
include:
- env: TOXENV=py27-django111
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I recently learned this syntax can be cleaned up like so:

matrix:
  include:
    - { python: 2.7, env: TOXENV=py27-django111 }
    - { python: 3.4, env: TOXENV=py34-django111 }
    - { python: 3.5, env: TOXENV=py35-django111 }
    - { python: 3.6, env: TOXENV=py36-django111 }
    ...

ref: encode/django-rest-framework/pull/5519

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx @rpkilby for the suggestion.

@dfunckt what do you think ?

Copy link
Owner

Choose a reason for hiding this comment

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

@ticosax @rpkilby yeah, that is readable, let's do it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, done.

And drop django versions lower than 1.11
@dfunckt
Copy link
Owner

dfunckt commented Oct 28, 2017

@ticosax I intend to include this in rules 2.0 — is this a problem for you? Is waiting for this PR blocking you in any way?

@blueyed
Copy link

blueyed commented Oct 28, 2017

@dfunckt
The question is not clear to me: if you want to include it, then I assume you would merge it?
AFAIK @ticosax might not respond until Wednesday, and it should be OK to merge it - if that is your question?!

@dfunckt
Copy link
Owner

dfunckt commented Oct 28, 2017

Hey @blueyed sorry I wasn't clear.

I'd like to include this PR in rules 2.0 because it contains breaking changes (dropping support for a bunch of Python and Django versions). But I'd also want to include a few other breaking changes in rules 2.0 (see #54) that aren't ready yet, which means this PR won't make it in a release that soon.

My question therefore is whether you're blocked on having a version of rules that has Django 2.0 support (i.e. having this PR merged), because I might then go ahead and merge this, release rules 2.0 and move the other breaking changes I wanted to include in another major version (i.e. 3.0) somewhat later. Makes sense?

@blueyed
Copy link

blueyed commented Oct 28, 2017

Oh, could have guessed it from your first comment that you do not plan to release 2.0 soonish.

I think it would make sense to have the Django 2.0 support separate, i.e. a release with fixes for Django 2.0 (extracted out of this PR), but then only later drop support for older versions?!
This might not be trivial though?!

In general it would be nice to have a release that is ready for Django 2.0, of course.

@dfunckt
Copy link
Owner

dfunckt commented Oct 28, 2017

That is a good plan and I actually considered it but on quick inspection thought it wouldn't be trivial as well.

But on second look, the only non-trivial change seems to be the templatetag registration decorator, because register.simple_tag exists in Django < 2.0 as well (but with different semantics than we need?) so you possibly can't just do:

try:
  register_tag = register.simple_tag
except AttributeError:
  # Compatibility with Django < 2.0
  register_tag = register.assignment_tag

@register_tag
def test_rule ...

Edit: actually, the imports above can be the other way round and it should work fine.

Other than that, the other changes are:

  1. A shim for MIDDLEWARE_CLASSES:
MIDDLEWARE = (
  # ...
)
# Compatibility with Django < 2.0
MIDDLEWARE_CLASSES = MIDDLEWARE
  1. A shim for include:
try:
  # Compatibility with Django < 2.0
  from django.conf.urls import include
except ImportError:
  include = lambda x: x

If this PR was tweaked to include these changes (dropping the changes to tox.ini, etc as well) I guess it'd be fine merge it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants