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

replace django.core.urlresolvers with django.urls to appeace Django2.… #106

Merged
merged 3 commits into from
Dec 11, 2017

Conversation

s-wirth
Copy link
Contributor

@s-wirth s-wirth commented Dec 8, 2017

…0 deprecations

@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage decreased (-0.6%) to 90.778% when pulling b4f5d27 on s-wirth:master into b2490e4 on kencochrane:master.

@@ -13,7 +13,7 @@
from django.contrib.auth.models import User
from django.contrib.auth.models import AnonymousUser
from django.contrib.sessions.backends.db import SessionStore
from django.core.urlresolvers import reverse
from django.urls import reverse
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be failing when run against old versions of django. Maybe we can do the following.

try:
    django.urls import reverse
except ImportError:
    django.core.urlresolvers import reverse

It tries to do it the new way, if it fails it falls back to the old way.

@@ -1,6 +1,6 @@
from django.shortcuts import render
from django.http import HttpResponseRedirect
from django.core.urlresolvers import reverse
from django.urls import reverse
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Collaborator

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, the changes are breaking some of the tests on older versions of django. If we can add the try except block as I described this should be able to make it backwards compatible.

@kencochrane
Copy link
Collaborator

I also noticed a similar change in #105 except your PR includes the tests.py as well.

@s-wirth
Copy link
Contributor Author

s-wirth commented Dec 11, 2017

@kencochrane Thank you very much for the comments! I also looked at the other PR you mentioned and added the updated install_requirements to my PR setup.py. Hope it works now :)

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.07%) to 91.429% when pulling 2cedcda on s-wirth:master into b2490e4 on kencochrane:master.

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.07%) to 91.429% when pulling ad0700b on s-wirth:master into b2490e4 on kencochrane:master.

Copy link
Collaborator

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@kencochrane kencochrane merged commit cde53c5 into jazzband:master Dec 11, 2017
@btoueg
Copy link
Contributor

btoueg commented Jan 11, 2018

I see that change requests are not the same for everyone ;)

Testing against Django 2.0 has not been added here (see #105)

@kencochrane
Copy link
Collaborator

@btoueg this PR was made after yours. Your PR already had those changes. I made the mistake and assumed your PR would be merged first and didn’t want to have duplicate code in more than one PR, to limit merge conflicts. Sorry for the trouble.

@btoueg btoueg mentioned this pull request Feb 1, 2018
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.

None yet

4 participants