-
Notifications
You must be signed in to change notification settings - Fork 104
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
PR for #57 and #58 #59
Conversation
catchup
Can you rebase on master? ;) |
There is something wrong with Python3, see tests: https://travis-ci.org/jedie/django-reversion-compare/builds/106334934 |
Can do, not sure what happened there. |
There is something screwy going on with the reversion 1.9 build, I'm still working on this, but thoughts are welcome. |
I'd suggest we keep Django==1.8 and Reversion=1.10.1 in the matrix. Django 1.8 is the long term support branch, and is compatible with 1.10.1 (but not 1.10). |
Aha!! It seems that Python3 was running the tests in a different order, and one of the tests was unregistering and reregistering a model, but without the follows. In one order this was ok, in another it failed. Should be ok now! |
No, i mean reversion 1.9 and 1.10 with django 1.8 and django 1.9 under python 2 and 3 ;) |
- DJANGO='1.9' REVERSION='1.10' EXTRA='' | ||
- DJANGO='1.8' REVERSION='1.9' EXTRA='diff-match-patch' | ||
- DJANGO='1.8' REVERSION='1.10.1' EXTRA='diff-match-patch' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we explicit test with 1.10.1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I read that Django 1.8 isn't compatible with 1.10, but I appear to have been mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, i don't know...
We test these combinations:
django 1.8.* with reversion 1.9.*
django 1.9.* with reversion 1.10.*
And all this with Python 2.7, 3.4 and 3.5
see here: https://travis-ci.org/jedie/django-reversion-compare/builds/106359851
Maybe it makes sense to add Django 1.8 with reversion 1.10.*
On the other side: This is more a job for reversion itself, isn't it? They run tests with older django versions too: https://travis-ci.org/etianen/django-reversion/builds/105372317
Think the current test matrix is fine, isn't it?
"bool1": obj_compare.value1, | ||
"bool2": obj_compare.value2, | ||
"bool1": str(obj_compare.value1), | ||
"bool2": str(obj_compare.value2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is str()
really needed? Why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No its not, I've previously had troubles with None in templates, but this isn't an issue now.
Code review noted, will fix changes in the morning. |
I've updated the boolean compare functions as per the code review :) I've also tweaked the travis matrix from:
to
Its probably overzealous, but given that the tests are rather quick*, and 1.8 will be supported until 2018, I think an explcit test of django 1.8 with the latest reversion is worth doing. * As an interesting aside, the full test suite ran before I finished typing this up :) |
That's fine! I done the merge. Thanks for your work. |
I will release v0.6.0 today. But currently PyPi is off: https://status.python.org/ |
No worries, this is a really useful module. So I'm glad to give back. |
Now v0.6.0 is on PyPi ;) |
No description provided.