Prepare for Django 2.0 #320

flesser · 2017-10-20T06:37:06Z

I was trying to get django-simple-history working with Django 2.0.
Here are the changes I made to get it run. (No guarantee for completeness or correct downward compatibility)

`field.rel.to` was deprecated since 1.9 and removed in 2.0

`django.core.urlresolvers` was deprecated since 1.10 and removed in 2.0 see https://docs.djangoproject.com/en/1.11/ref/urlresolvers/

codecov-io · 2017-10-20T06:42:07Z

Codecov Report

Merging #320 into master will decrease coverage by 0.59%.
The diff coverage is 78.26%.

@@            Coverage Diff            @@
##           master     #320     +/-   ##
=========================================
- Coverage   97.63%   97.04%   -0.6%     
=========================================
  Files          15       15             
  Lines         593      609     +16     
  Branches       73       74      +1     
=========================================
+ Hits          579      591     +12     
- Misses          7       10      +3     
- Partials        7        8      +1

Impacted Files	Coverage Δ
...istory/registry_tests/migration_test_app/models.py	`100% <100%> (ø)`	⬆️
simple_history/admin.py	`98.09% <100%> (+0.05%)`	⬆️
simple_history/models.py	`95.16% <72.22%> (-1.74%)`	⬇️

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 35febc3...c8e09e8. Read the comment docs.

`is_authenticated` has been a method until Django 1.9. Further information: https://docs.djangoproject.com/en/1.11/releases/1.10/#using-user-is-authenticated-and-user-is-anonymous-as-methods

Updated `is_authenticated` to be a property.

merwok · 2017-11-29T21:43:02Z

simple_history/models.py

@@ -173,7 +173,8 @@ def copy_fields(self, model):
                if getattr(old_field, 'db_column', None):
                    field_arguments['db_column'] = old_field.db_column
                field = FieldType(
-                    old_field.rel.to,
+                    # required for Django <= 1.8                     # required for Django >= 2.0
+                    old_field.rel.to if hasattr(old_field, 'rel') else old_field.remote_field.model,


Having this inline makes for a dense line IMHO.

Suggest, for readability

if hasattr(old_field, 'rel'): target = old_field.rel.to # Django <= 1.8 else: target = old_field.remote_field.model # Django >= 2.0 field = FieldType(target, ...

This could even be determined just once and saved (in this module before this class, or in a shared compat module).

Also I think the test should be for the new attribute name; both exists in django 1.11 but the old-style one causes a warning.

merwok · 2017-11-29T21:43:26Z

simple_history/models.py

-                if self.thread.request.user.is_authenticated():
+                try:
+                    # Django < 1.10
+                    is_authenticated = self.thread.request.user.is_authenticated()


This will work but emit warnings in 1.10+

Note: In Django 2.0 the AttributeError is raised in all passing tests right now.

@dgilge

Further fixes for Django 2.0 contributed by @dgilge

merwok · 2017-12-04T16:13:44Z

simple_history/models.py

            except AttributeError:
                return None
+            if not is_authenticated in (True, False):
+                is_authenticated = is_authenticated() # Django < 1.10


I think this is not correct. In 1.10 or 1.11, is_authenticated is an instance of CallableTrue or CallableFalse, not True or False, so the if not in test will fail and go into line 283 which will raise the deprecation warning.

Yes, that might be. I tested it having the callable boolean in mind but now I saw that none of the implemented tests get to this line at all in Django 1.10+ (because self.thread.request.user.is_authenticated raises the caught AttributeError.

request.user raises an AttributeError in multiple tests in Django 2.0 (AttributeError: 'WSGIRequest' object has no attribute 'user'). For that reason it might be the next step to fix those.

This error might be caused by a wrong ordering of middleware.

RossRogers · 2017-12-20T19:32:32Z

simple_history/tests/urls.py

-    url(r'^admin/', include(admin.site.urls)),
-    url(r'^other-admin/', include(other_admin.site.urls)),
+    url(r'^admin/', admin.site.urls),
+    url(r'^other-admin/', other_admin.site.urls),


Is the omission of function include backwards compatible? All other edits seem backwards compatibility except maybe this one.

The tests passed using tox.

RossRogers · 2017-12-20T19:54:42Z

@treyhunner , @macro1 ,

If you simply inspect this pull's edits, they are, at the very least, non-destructive and backwards compatible to my eyes. Any chance of merging this pull request and releasing django-simple-history ?

merwok · 2017-12-20T19:57:40Z

PR has a conflict to resolve before it can be merged.

RossRogers · 2017-12-20T22:37:53Z

Ok. I forked @treyhunner's repor and pulled in @flesser 's repo here https://github.com/RossRogers/django-simple-history . This is my merge resolution for the only merge conflict:

-                 if isinstance(old_field.rel.to, str) and old_field.rel.to == 'self':
++                
++                #           required for Django <= 1.8                       # required for Django >= 2.0
++                object_to = old_field.rel.to if hasattr(old_field, 'rel') else old_field.remote_field.model
++                if isinstance(object_to, str) and object_to == 'self':
 +                    object_to = old_field.model
-                 else:
-                     object_to = old_field.rel.to

@treyhunner , @macro1 ,

Do you want me to create another pull request, superseding this one?

Django2.0

cgthayer

Thanks for fixing this! Looking forward to being able to use Django 2.0

cgthayer · 2017-12-21T15:12:00Z

simple_history/models.py

@@ -179,7 +179,8 @@ def copy_fields(self, model):
                if isinstance(old_field.rel.to, str) and old_field.rel.to == 'self':
                    object_to = old_field.model
                else:
-                    object_to = old_field.rel.to
+                    #           required for Django <= 1.8                       # required for Django >= 2.0


minor: doubled up comment

merwok · 2017-12-21T17:49:21Z

simple_history/models.py

@@ -179,7 +179,8 @@ def copy_fields(self, model):
                if isinstance(old_field.rel.to, str) and old_field.rel.to == 'self':
                    object_to = old_field.model
                else:
-                    object_to = old_field.rel.to
+                    #           required for Django <= 1.8                       # required for Django >= 2.0
+                    object_to = old_field.rel.to if hasattr(old_field, 'rel') else old_field.remote_field.model


As said before, this is the wrong order to avoid warnings.

Line 179 seems to have the same problem.

So, you want to see
old_field.remote_field.model if hasattr(old_field, 'remote_field') else old_field.rel.to
?

Did that edit plus helped out @flesser 's merge to guard new uses of old_field.rel.to here https://github.com/flesser/django-simple-history/pull/4 .

@merwok

…order field check per @merwok feedback

@merwok

Correct merge to remove new uses of `old_field.rel.to`. Reorder field check per @merwok feedback

RossRogers · 2018-01-24T19:52:13Z

Well... I'm trying to do the migration, but having trouble with tests, code coverage, and environments. My codebase is running fine against this version, but I have a niche use case where I use it to talk to IBM's DB2, and antique windows SQL Server instances from the turn of the millenia:

https://github.com/RossRogers/django-pyodbc

You can install with

pip install git+ssh://[email protected]/RossRogers/django-pyodbc@master

So, I'm in the middle of doing the upgrade to django 2.0 myself and then I want to create a new django 2.0 branch and upgrade the major version to 2, get code coverage up, create release candidates for the pip system et c. Anyways, still a work in progress, but you're welcome to try the version I'm using. May want to fork it though, as I need to move those edits to a new branch. My repo is a bit of a scratch repo...

Rested · 2018-01-25T13:39:59Z

@RossRogers Right, im currently using your https://github.com/RossRogers/django-simple-history/archive/django2.0.zip#egg=django-simple-history and it seems to be working. Would just be great if this could be merged and pushed to pip. What is blocking that at the moment @merwok ? Coverage?

merwok · 2018-01-25T16:09:32Z

I’m not in a position to block a PR in this repo, just making comments since I’m also using django-simple-history :)

My comments were about wrong code for is_authenticated (would still raise warnings as is since that attribute won’t be True/False but a CallableTrue/CallableFalse) and code style (the repeated super long lines for the related attributes).

RossRogers · 2018-01-25T16:14:31Z

Oh my goodness. I was commenting in the repository. I've been trying to migrate django-pyodbc. Apologies.

ransford · 2018-02-07T22:22:43Z

Hi! My little group of django-simple-history fans can offer to help here if the tasks are well defined, such as writing new tests to increase coverage. Is that the only thing remaining before this is merged, or do the is_authenticated issues @dgilge and @merwok raised need to be addressed via code fixes, too?

RossRogers · 2018-02-07T23:53:10Z

If I may be so presumptuous, I think activity from the maintainers/committers is kinda what's missing. If so, I totally get that life happens.

ransford · 2018-02-08T00:09:11Z

Sorry @RossRogers, I assumed you were a maintainer because of the diligent work you already put in -- hat tip to you.

pikrzysztof · 2018-02-09T13:30:32Z

Can someone comment on branch-20 health and which commits are best to use?

RossRogers · 2018-02-09T15:42:08Z

@pikrzysztof , all the edits are necessary... If you look, there are not many file diffs. Most of the edits come from pulling from the master repository and doing an automerge. Anyways, I'm using my clone in production and I haven't had any problems, yet..

 pip install -U git+ssh://[email protected]/RossRogers/[email protected]

jhrr · 2018-02-09T15:58:35Z

Prob needs to be pip install -e git+https://[email protected]/RossRogers/[email protected]#egg=django_simple_history though.

RossRogers · 2018-02-09T16:05:28Z

Maybe that works too, but I've installed it with that other command successfully in 3 different virtualenvs, both dev and production.

jhrr · 2018-02-09T16:06:15Z

I don't think it will work for other people as they don't share your ssh credentials.

RossRogers · 2018-02-09T16:11:33Z

The repo is wide open to reads, including using your credentials, but ok, if you don't have ssh credentials for your account on your github account, then it will fail that ssh command. Anyways, [unfortunately] many ways to skin a cat in git.

jhrr · 2018-02-09T16:13:30Z

I do have ssh creds for my account and your command did fail with permission denied.

RossRogers · 2018-02-09T16:14:43Z

Okey doke.

Rested · 2018-02-09T18:12:39Z

pip install https://github.com/RossRogers/django-simple-history/archive/django2.0.zip#egg=django-simple-history Should work fine

…

On 9 Feb 2018, 17:13 +0100, jhrr ***@***.***>, wrote: I do have ssh creds for my account and your command did fail with permission denied. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

KevinGrahamFoster · 2018-02-21T19:24:32Z

I'm available to help make this PR a released reality too if someone gives some direction in what's required both from a code standpoint and for the two failing checks. It appears to just be too big of a diff in coverage, suggesting that we should add more coverage tests. Can someone confirm that?

mattLLVW · 2018-03-15T12:58:25Z

Any update on this release?

merwok · 2018-03-15T13:26:59Z

I don’t know if codecov supports combining the coverage data from multiple runs. With compat branches, you have to run tests with different Python and Django versions to go into all branches.

tox and coverage.py make it pretty easy to do that locally, but I don’t know how much control Travis / Codecov give you here.

macro1 · 2018-03-18T00:51:04Z

@merwok the tests were not actually running against Django 2.0. I opened a new PR just now that does have the test changes as well (#353)

macro1 · 2018-03-18T01:12:28Z

Interesting... Apparently Github will combine upstream changes to a downstream branch in a PR

rossmechanic · 2018-04-05T18:53:35Z

Merged necessary changes into master. Support for Django2.0 should now work on master. Will release soon. Added the authors here into AUTHORS.rst to give you al credit

dgilge · 2018-04-06T08:40:37Z

Great! Thanks for maintaining this valuable package!

flesser added 3 commits October 20, 2017 08:30

Update models.py for Django 2.0

7cc283e

`field.rel.to` was deprecated since 1.9 and removed in 2.0

Update admin.py for Django 2.0

32f2dc2

`django.core.urlresolvers` was deprecated since 1.10 and removed in 2.0 see https://docs.djangoproject.com/en/1.11/ref/urlresolvers/

Swap imports

0d73d84

merwok mentioned this pull request Nov 7, 2017

Address warnings from Django 1.11 #291

Closed

dgilge and others added 3 commits November 28, 2017 16:27

Updated is_authenticated to be a property.

48147a5

`is_authenticated` has been a method until Django 1.9. Further information: https://docs.djangoproject.com/en/1.11/releases/1.10/#using-user-is-authenticated-and-user-is-anonymous-as-methods

Merge pull request #1 from dgilge/django2.0

e36a105

Updated `is_authenticated` to be a property.

Improved implementation for is_authenticated

38c72ca

merwok reviewed Nov 29, 2017

View reviewed changes

hangya mentioned this pull request Dec 3, 2017

Django 2.0 compatibility #330

Closed

dgilge and others added 5 commits December 4, 2017 09:02

is_authenticated shouldn't emit a warning in Django 1.10+ anymore.

41c5965

Note: In Django 2.0 the AttributeError is raised in all passing tests right now.

Added on_delete to some test model fields

212dd35

Swap import in test

49d53d4

Updated urls in tests

07b206c

Merge pull request #2 from dgilge/django2.0

b5a32b9

Further fixes for Django 2.0 contributed by @dgilge

merwok reviewed Dec 4, 2017

View reviewed changes

giovanisleite mentioned this pull request Dec 4, 2017

Scheduled weekly dependency update for week 49 okfn-brasil/jarbas#295

Merged

cuducos mentioned this pull request Dec 5, 2017

Update do Django 2.0 okfn-brasil/jarbas#296

Open

3 tasks

jmbowman mentioned this pull request Dec 6, 2017

Handle reverse move and models.permalink removal #331

Closed

RossRogers reviewed Dec 20, 2017

View reviewed changes

Merge fix for Django 2.0 with new self-referential model logic

5622e85

Merge pull request #3 from RossRogers/django2.0

701a815

Django2.0

cgthayer reviewed Dec 21, 2017

View reviewed changes

merwok reviewed Dec 21, 2017

View reviewed changes

Ross Rogers and others added 2 commits December 21, 2017 10:12

Correct @flesser's merge to remove new uses of old_field.rel.to. Re…

bd58b70

…order field check per @merwok feedback

Merge pull request #4 from RossRogers/django2.0

a75adab

Correct merge to remove new uses of `old_field.rel.to`. Reorder field check per @merwok feedback

anapaulagomes mentioned this pull request Jan 26, 2018

Update to Django 2.0 okfn-brasil/jarbas#311

Open

1 task

idlesign mentioned this pull request Mar 10, 2018

Перейти на Django 2.0 idlesign/pythonz#28

Closed

2 tasks

Clean up changes for Django 2.0, update tests

2fe4038

macro1 mentioned this pull request Mar 18, 2018

Add support for Django 2.0 #353

Closed

Update AUTHORS and CHANGES

c8e09e8

rossmechanic mentioned this pull request Apr 3, 2018

Dropping Support for Django < 1.11 #356

Closed

rossmechanic closed this Apr 5, 2018

Prepare for Django 2.0 #320

Prepare for Django 2.0 #320

Conversation

flesser commented Oct 20, 2017

codecov-io commented Oct 20, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgilge Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossRogers commented Dec 20, 2017

merwok commented Dec 20, 2017

RossRogers commented Dec 20, 2017 • edited Loading

cgthayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossRogers commented Jan 24, 2018 • edited Loading

Rested commented Jan 25, 2018

merwok commented Jan 25, 2018

RossRogers commented Jan 25, 2018

ransford commented Feb 7, 2018

RossRogers commented Feb 7, 2018

ransford commented Feb 8, 2018

pikrzysztof commented Feb 9, 2018

RossRogers commented Feb 9, 2018

jhrr commented Feb 9, 2018 • edited Loading

RossRogers commented Feb 9, 2018 • edited Loading

jhrr commented Feb 9, 2018

RossRogers commented Feb 9, 2018

jhrr commented Feb 9, 2018

RossRogers commented Feb 9, 2018

Rested commented Feb 9, 2018 via email

KevinGrahamFoster commented Feb 21, 2018

mattLLVW commented Mar 15, 2018

merwok commented Mar 15, 2018

macro1 commented Mar 18, 2018

macro1 commented Mar 18, 2018

rossmechanic commented Apr 5, 2018

dgilge commented Apr 6, 2018

codecov-io commented Oct 20, 2017 •

edited

Loading

dgilge Dec 20, 2017 •

edited

Loading

RossRogers commented Dec 20, 2017 •

edited

Loading

RossRogers commented Jan 24, 2018 •

edited

Loading

jhrr commented Feb 9, 2018 •

edited

Loading

RossRogers commented Feb 9, 2018 •

edited

Loading