Add django 2.0 support #318

meshy · 2017-10-29T00:34:53Z

This fixes a few minor issues to add support for django 2.0(b1).

I have:

Removed django 1.10 from the test suite in the process, ~~but have not yet dug out compat fixes that were added for it~~.
Removed three models from the tests that were never referenced in the tests.
Not yet tested this manually.

There is one remaining unaddressed RemovedInDjango20Warning, but I've had some frustration with it. See https://code.djangoproject.com/ticket/28750

TODO:

Remove django 1.10 compat fixes.
Chase up related django ticket.

codecov · 2017-11-03T23:07:52Z

Codecov Report

Merging #318 into master will increase coverage by 0.22%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   87.75%   87.98%   +0.22%     
==========================================
  Files          33       33              
  Lines        2483     2471      -12     
==========================================
- Hits         2179     2174       -5     
+ Misses        304      297       -7

Impacted Files	Coverage Δ
polymorphic/tests/models.py	`100% <ø> (ø)`	⬆️
polymorphic/tests/admintestcase.py	`92.3% <ø> (ø)`	⬆️
polymorphic/models.py	`93.87% <100%> (+1.79%)`	⬆️
polymorphic/formsets/utils.py	`100% <100%> (ø)`	⬆️
polymorphic/tests/test_orm.py	`100% <100%> (+1.06%)`	⬆️
polymorphic/admin/parentadmin.py	`71.82% <80%> (+0.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fe7861...ed40b9e. Read the comment docs.

meshy · 2017-11-05T22:30:10Z

tox.ini

+	django200: Django ~= 2.0b1
 	djangomaster: https://github.com/django/django/archive/master.tar.gz
 	postgres: psycopg2
+pip_pre = True


This will not be required once django 2.0 comes out. Also, the line above will need to change to Django ~= 2.0.0

EDIT: If django 2.0 is released before this branch is merged, I'll fix this in a rebase.

This can be updated to Django 2.0rc1

We can now:

remove pip_pre line

django200: Django >= 2.0, < 2.1

meshy · 2017-11-11T16:22:51Z

I'm not able to reproduce the python 2.7 failure locally, unfortunately, so I'd appreciate any insight on the matter.

vdboor

great work, thanks a lot!
The add_media() needs some improvement, the rest looks ok!

vdboor · 2017-11-20T13:32:13Z

polymorphic/compat.py

@@ -0,0 +1,10 @@
+import django
+
+DJANGO_GTE_2 = django.VERSION >= (2, 0)


I'd place django.VERSION >= (2, 0) or even better: django.VERSION < (2, 0) directly inline in the code

Sure ok. WRT this and your other comment in this compat file, please excuse me, I was working on habit here. I've found that it's helpful to have a single point of reference for compatibility hacks, as it makes maintenance easier down the road. I probably shouldn't have introduced it here without running it past you first though :)

I'll move this inline, as you have suggested.

vdboor · 2017-11-20T13:32:46Z

polymorphic/compat.py

+    from django.urls import URLResolver
+except ImportError:
+    # Django < 2.0
+    from django.urls import RegexURLResolver as URLResolver


There is only a single place where this import is needed. Does that quality for a separate file to do this?

vdboor · 2017-11-20T13:33:45Z

tox.ini

+	django200: Django ~= 2.0b1
 	djangomaster: https://github.com/django/django/archive/master.tar.gz
 	postgres: psycopg2
+pip_pre = True


This can be updated to Django 2.0rc1

vdboor · 2017-11-20T13:34:13Z

polymorphic/models.py


    class Meta:
        abstract = True
        base_manager_name = "objects"


do we need a default_manager_name too?

vdboor · 2017-11-20T13:34:41Z

polymorphic/models.py

    class Meta:
        abstract = True
        base_manager_name = "objects"
+        if not DJANGO_GTE_2:


if django.VERSION < (2, 0) would be clean here too.

vdboor · 2017-11-20T13:37:57Z

polymorphic/formsets/utils.py

-    dest.add_css(media._css)
-    dest.add_js(media._js)
+    if DJANGO_GTE_2:
+        dest += media


⚠️ This no longer updates the dest object so it will break. Returning dest and updating polymorphic/admin/inlines.py, polymorphic/formsets/models.py would be better.

Are you 100% sure this fails? I thought I had tested this quite thoroughly.

If you're sure you'd like this to change, please confirm, and I'll get it done.

tony · 2017-12-06T16:55:39Z

I also have a branch I've started on my own at https://github.com/develtech/django-polymorphic/tree/django-2.0.

I'm going to consider closing #327 and #328 in favor of moving this one along.

tony · 2017-12-06T17:02:32Z

.travis.yml

      env: TOXENV=py35-django111
+    - python: "3.6"
+      env: TOXENV=py35-django200
    - python: "3.6"


tony · 2017-12-06T17:02:44Z

@meshy Can you swing around to removing the compat module/inlining version checks?

tony · 2017-12-06T17:07:41Z

https://travis-ci.org/django-polymorphic/django-polymorphic/jobs/300629621

I don't know where that is coming from. Here's the recreation:

git remote add meshy https://github.com/meshy/django-polymorphic.git
git fetch meshy
git checkout meshy/django-2.0
tox

Everything greenlights except for Python 2.7 + Django 1.11

tox -e py27-django111

Response:

LOB sdist-make: /Users/me/work/python/django-polymorphic/setup.py
py27-django111 inst-nodeps: /Users/me/work/python/django-polymorphic/.tox/dist/django-polymorphic-1.3.zip
py27-django111 installed: coverage==4.4.2,dj-database-url==0.4.2,Django==1.11.8,django-polymorphic==1.3,pytz==2017.3
py27-django111 runtests: PYTHONHASHSEED='1573255718'
py27-django111 runtests: commands[0] | coverage run --source polymorphic runtests.py
Using Python version 2.7.1 from /Users/me/work/python/django-polymorphic/.tox/py27-django111/bin/python2.7
Using Django version 1.11.8 from /Users/me/work/python/django-polymorphic/.tox/py27-django111/lib/python2.7/site-packages/django
Creating test database for alias 'default'...
Creating test database for alias 'secondary'...
System check identified no issues (0 silenced).
=====================================================================
FAIL: test_defer_fields (polymorphic.tests.test_orm.PolymorphicTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/me/work/python/django-polymorphic/polymorphic/tests/test_orm.py", line 227, in test_defer_fields
    '<Model2A: id 1, field1 (CharField), deferred[field1]>')
AssertionError: '<Model2A: id 5, field1 (CharField), deferred[field1]>' != '<Model2A: id 1, field1 (CharField), deferred[field1]>'

----------------------------------------------------------------------
Ran 57 tests in 9.678s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'secondary'...
ERROR: InvocationError: '/Users/me/work/python/django-polymorphic/.tox/py27-django111/bin/coverage run --source polymorphic runtests.py'

tony · 2017-12-06T17:49:54Z

Is the ID of the model an important detail to test against when using defer()?

https://docs.djangoproject.com/en/1.11/ref/models/querysets/#django.db.models.query.QuerySet.defer

It is strange that id isn't acting as expected. I got no idea where that's coming from.

Flipping around sqlite addresses:

diff --git a/runtests.py b/runtests.py
index bb3074d..810a58e 100755
--- a/runtests.py
+++ b/runtests.py
@@ -19,14 +19,14 @@ if not settings.configured:
     settings.configure(
         DEBUG=False,
         DATABASES={
-            'default': dj_database_url.config(
-                env='PRIMARY_DATABASE',
-                default='sqlite://:memory:',
-            ),
-            'secondary': dj_database_url.config(
-                env='SECONDARY_DATABASE',
-                default='sqlite://:memory:',
-            ),
+            'default': {
+                'ENGINE': 'django.db.backends.sqlite3',
+                'NAME': 'file:mydatabase2.db',
+            },
+            'secondary': {
+                'ENGINE': 'django.db.backends.sqlite3',
+                'NAME': 'file:mydatabase44.db',
+            },
         },
         TEST_RUNNER="django.test.runner.DiscoverRunner",
         INSTALLED_APPS=(

Changed nothing. It doesn't seem to be anything related to secondary db (same result happens if second db removed from settings).

I also ruled out it being due to other tests inside the same class (which shouldn't matter anyway if its in a TransactionTestCase)

I'm not sure if the ID is as important as the field showing up deferred.

tony · 2017-12-06T18:01:12Z

OK, I've ruled out the above error not being related to this PR, it started here 80b4f2b.

So other than the bikeshed stuff (removing the compat file, updating versions and the pip_pre) this is good for another look.

meshy · 2017-12-11T22:46:48Z

@tony thank you for the investigation into the failing test. It's a great relief to know that the cause does not stem from this PR.

meshy · 2017-12-11T23:09:53Z

I've made most of the requested changes here, and have rebased the commits. Still waiting on a confirmation on the changes requested to polymorphic/formsets/utils.py before making a significant change there.

I realise it's taken me quite a while to get back to this one -- thank you for your patience!

tony · 2017-12-18T14:59:43Z

I closed my #327 and #328 because they duplicate the changes here.

meshy · 2018-01-05T13:08:50Z

This is blocked because of failing tests. I believe the issue is in the master branch. See #331.

vdboor · 2018-01-15T14:04:59Z

The fix in #322 is merged! Sorry for the delay. Would this clear the way for Django 2.0 now?

You may want to try rebasing the branch

Because the URL mechanism has changed in Django 2.0, the RegexURLResolver has been renamed.

The old format was deprecated in Django 1.9, and removed in Django 2.0. Ref: https://docs.djangoproject.com/en/stable/releases/1.9/#passing-a-3-tuple-or-an-app-name-to-include

Neither `add_css` nor `add_js` exist in Django 2.0 because the method for adding `Media` classes together has changed. Ref: django/django@c19b56f

This silences one instance of the warning that's being printed in tests for versions of Django before 2.0: > RemovedInDjango20Warning: Managers from concrete parents will soon > qualify as default managers if they appear before any other managers > in the MRO.

meshy · 2018-01-16T11:12:06Z

Thanks for merging that. I'm still getting that one test failure, but I should be able to investigate it now. It's frustrating that I'm not getting the failure in local tests; only on Travis.

meshy · 2018-01-16T16:11:53Z

@tony did you get to the bottom of what might be causing the issue?

tony · 2018-01-16T16:16:59Z

(I have a lot on my plate ATM, and am not using django-polymorphic in my sites anymore.)

I think that deferred issue started happening in 80b4f2b. I don't think it's related to this PR.

I notice that in https://github.com/django-polymorphic/django-polymorphic/blob/80b4f2bb41423d6131e2e39ef5c6b212ce5a7b56/polymorphic/tests/test_orm.py#L232, the asserts weren't updated to be wrapped in repr.

tony · 2018-01-16T16:17:56Z

See #332 for the last test failure. But I think this PR is ok.

meshy · 2018-01-16T16:19:11Z

@tony thank you for taking the time to help out here, especially given you've no projects using this library. I quite understand.

That looks like a great place for me to dig in. Thanks again!

Sometimes the tests failed because these objects had IDs that differed from expectations. As the IDs are not relevant to this test, I have replaced the exact string match with a regex match that accepts any ID.

WhyNotHugo · 2018-01-17T22:04:00Z

polymorphic/tests/test_orm.py

-                         '<Model2C: id 3, field1 (CharField), field2 (CharField), field3 (CharField), deferred[field1]>')
-        self.assertEqual(repr(objects_deferred[3]),
-                         '<Model2D: id 4, field1 (CharField), field2 (CharField), field3 (CharField), field4 (CharField), deferred[field1]>')
+        self.assertRegex(repr(objects_deferred[0]),


Honestly, this could really be an assertQuerySetEqual, which is more future-proof, and quite cleaner.

Not a blocker for this specific PR though.

I'm not really sure how I would make that change. Thanks for not blocking on it!

meshy · 2018-01-17T22:13:51Z

Tests now appear to be passing! \o/

vdboor

Wow, this change is really getting in the right shape!

The only nagging thing that remains is the add_media performance hack. The proposed solution does break in Django 2.0.

vdboor · 2018-01-18T14:40:45Z

polymorphic/formsets/utils.py

-    dest.add_css(media._css)
-    dest.add_js(media._js)
+    if django.VERSION >= (2, 0):
+        dest += media


This will break. The Django code for dest += does the following:

def __add__(self, other): combined = Media() combined._js = self.merge(self._js, other._js) combined._css = { medium: self.merge(self._css.get(medium, []), other._css.get(medium, [])) for medium in self._css.keys() | other._css.keys() } return combined

Thus, the dest object that was passed to the function is not changed here, while that was the whole point of the add_media performance boost.

However, that was easy to fix, so I've done that in 9d651a1 :-)

It took me far too long to understand why it didn't work. Thanks for fixing that! :)

vdboor · 2018-01-18T14:51:41Z

I'm super happy to see all the collaboration to get this PR working. If one of you guys like to have a commit-bit for this project, you're more then welcome! Just mention it here!

Having more people to take up work only seems healthy to me :-)

mblayman · 2018-01-19T02:56:47Z

Kudos to you all for getting Django 2.0 support added!

I'm the maintainer for Django REST Framework JSON API (https://github.com/django-json-api/django-rest-framework-json-api), and its test suite relies on django-polymorphic for some polymorphic capable serializers that were added. I'm eagerly awaiting this project's next release so I can push out a Django 2.0 compatible release of DJA. 😄 Nice work!

meshy mentioned this pull request Oct 29, 2017

RegexURLResolver usage #314

Closed

meshy force-pushed the django-2.0 branch 2 times, most recently from ab81d04 to df17d57 Compare November 5, 2017 22:11

meshy mentioned this pull request Nov 5, 2017

Support Django 2.0 meshy/django-conman#109

Open

meshy commented Nov 5, 2017

View reviewed changes

meshy force-pushed the django-2.0 branch from df17d57 to d40f782 Compare November 11, 2017 15:26

vdboor requested changes Nov 20, 2017

View reviewed changes

tony reviewed Dec 6, 2017

View reviewed changes

.travis.yml

env: TOXENV=py35-django111

- python: "3.6"

env: TOXENV=py35-django200

- python: "3.6"

Copy link

Member

tony Dec 6, 2017

Choose a reason for hiding this comment

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

LGTM

tony mentioned this pull request Dec 6, 2017

Support for named :memory: urls? jazzband/dj-database-url#97

Closed

tony mentioned this pull request Dec 6, 2017

Python 2.7 + Django 1.11 breaking #332

Closed

tony mentioned this pull request Dec 6, 2017

Committing directly to master #333

Closed

meshy force-pushed the django-2.0 branch from d40f782 to 3f60855 Compare December 11, 2017 23:08

This was referenced Dec 18, 2017

Admin fieldsets fix (another attempt) #322

Merged

update tests + CI against django 2.0, fix #325 #328

Closed

update MIDDLEWARE variable #327

Closed

vdboor force-pushed the master branch from 02b71d8 to a2ba525 Compare December 22, 2017 09:03

meshy changed the title ~~WIP: Add django 2.0 support~~ Blocked: Add django 2.0 support Jan 9, 2018

mblayman mentioned this pull request Jan 9, 2018

Add DRF 3.7 and Django 2.0 to the test matrix. django-json-api/django-rest-framework-json-api#394

Merged

meshy added 11 commits January 16, 2018 10:53

Fix import in Django 2.0

840bfe8

Because the URL mechanism has changed in Django 2.0, the RegexURLResolver has been renamed.

Fix deprecated admin site URL in tests

1a4595f

The old format was deprecated in Django 1.9, and removed in Django 2.0. Ref: https://docs.djangoproject.com/en/stable/releases/1.9/#passing-a-3-tuple-or-an-app-name-to-include

Remove django 1.10 from tests

96b11b5

Add django 2.0 to tests

60e56ed

Fix add_media util for Django 2.0

120520e

Neither `add_css` nor `add_js` exist in Django 2.0 because the method for adding `Media` classes together has changed. Ref: django/django@c19b56f

Mention django 2.0 support in changelog

6fb34a0

Remove unused function in tests

3f6d941

Clean up unrequired compat import

ca13180

Update docs to reflect supported django versions

388d0e1

Update trove classifiers and django requirement

c317905

meshy force-pushed the django-2.0 branch from 3f60855 to c317905 Compare January 16, 2018 10:54

stasfilin mentioned this pull request Jan 16, 2018

Add Django 2.0 support django-polymorphic/django-polymorphic-tree#67

Merged

Use assertRegex to ignore object IDs

ed40b9e

Sometimes the tests failed because these objects had IDs that differed from expectations. As the IDs are not relevant to this test, I have replaced the exact string match with a regex match that accepts any ID.

WhyNotHugo reviewed Jan 17, 2018

View reviewed changes

meshy changed the title ~~Blocked: Add django 2.0 support~~ Add django 2.0 support Jan 17, 2018

vdboor requested changes Jan 18, 2018

View reviewed changes

vdboor merged commit e164026 into jazzband:master Jan 18, 2018

meshy deleted the django-2.0 branch January 18, 2018 14:50

		@@ -0,0 +1,10 @@
		import django

		DJANGO_GTE_2 = django.VERSION >= (2, 0)

Uh oh!

Add django 2.0 support #318

Add django 2.0 support #318

Uh oh!

Conversation

meshy commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

meshy Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshy commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdboor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tony commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tony commented Dec 6, 2017

Uh oh!

tony commented Dec 6, 2017

Uh oh!

tony commented Dec 6, 2017

Uh oh!

tony commented Dec 6, 2017

Uh oh!

meshy commented Dec 11, 2017

Uh oh!

meshy commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony commented Dec 18, 2017

Uh oh!

meshy commented Jan 5, 2018

Uh oh!

vdboor commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshy commented Jan 16, 2018

Uh oh!

meshy commented Jan 16, 2018

Uh oh!

tony commented Jan 16, 2018

Uh oh!

tony commented Jan 16, 2018

Uh oh!

meshy commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshy commented Oct 29, 2017 •

edited

Loading

codecov bot commented Nov 3, 2017 •

edited

Loading

meshy Nov 5, 2017 •

edited

Loading

meshy commented Nov 11, 2017 •

edited

Loading

meshy commented Dec 11, 2017 •

edited

Loading

vdboor commented Jan 15, 2018 •

edited

Loading