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

[WIP] Add Support for Django 1.11 and add Python 3 support. #27

Open
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

WTFox
Copy link

@WTFox WTFox commented Jan 23, 2018

Adds support for Python 3 and Django 1.11.

A high level overview

  • Removes the use of django.conf.urls.patterns since support was added for plain lists.
  • Replaces instances of unicode with str. Also adds python_2_unicode_compatible to Django models so that only __str__() has to be implemented.
  • Casts dict.keys() to a list since it's not that.
  • Removes instances of del dict[some_key] in iterations to avoid throwing a Iterable size changed during iteration error.
  • Adds a helper function to tests to make comparing lists of dicts more consistent.
  • Updates the project meta information.

Notes:

  • django.conf.urls.patterns support was removed in 1.8, so I've updated the readme to suggest using Django >= 1.8
  • This project is compatible with python >= 2.7 and python 3 >= 3.6.

Dangers with upgrading to Python 3

While this library works out of the box with Python 3 there is a caveat that could cause unexpected side effects. For that to happen the following conditions have to be true:

  • You have a GenericRelation to simple_audit.Audit from a Model that you've registered with Simple Audit
  • You're upgrading from Python 2 to Python 3

Lets say that a model contains a GenericRelation back to simple_audit.Audit so that the audits can be queried like instance.audits.all() and a new row gets added to the db. Simple Audit will mark that as an ADD operation and will create a record for it. It will look for all fields on the model and get the new and existing values for each one. It works as expected for all fields until it gets to our audits field, the GenericRelation.

What happens is that it looks for the value and calls unicode on it. The value in this case is a GenericRelationObjectManager instance, but calling str() on it will resolve it to simple_audit.Audit.None. This went largely unnoticed because the subsequent changes to the model would produce the same value for audits.

Python 3 comes in like a wrecking ball and obviously unicode() is no longer available. We have str() now for string types. Replacing the unicode usage with str does something entirely unexpected. Instead of resolving to the same simple_audit.Audit.None string, it now produces a string of the objects repr with memory locations e.g. '<django.contrib.contenttypes.fields.GenericRelatedObjectManager object at 0x10a72d510>'

This matters because when Simple Audit gets triggered, it gets the existing value and new instance value of all fields on the model - including fields that have a GenericRelation back to simple_audit.Audit. Which means there will always be a difference because of memory locations being printed in the string.

Long story short, upgrading to Python 3 can and probably will add extraneous information to your audit_change table and will muddy the information making it harder to digest.

binarymatt and others added 30 commits September 23, 2013 14:44
When loading fixtures, do not trigger audits.
Increased Auditchange old_value and new_value to textfield
Avoid errors when obj_description is > 100 chars.
Back-merge latest from original author.
…cal migrations) will be removed in Django 1.9.
Back-merge + fix Django warnings for 1.9.
Add Migrations & improve 1.9 compatibility
sburns and others added 21 commits March 1, 2016 15:09
Fix tests and update away from private _meta API usage
Update compatibility for Django 1.10
Because Django 1.10 removed model._meta.get_all_field_names,
it must be implemented as a model method. These changes are
backwards compatible with Django 1.8, 1.9

Refer to the docs to implement _get_all_field_names
as a model method or util function.
https://docs.djangoproject.com/en/1.10/ref/models/meta/#migrating-from-the-old-api
Modify to_dict for use with Django 1.10
Update version in setup so pip will find the most recent commit
Update version so pip will find the most recent commit
@WTFox
Copy link
Author

WTFox commented Jan 26, 2018

@leandrosouza Do you know when you'll be able to look at this? Not trying to push, just curious. Thanks for your help! :)

@WTFox
Copy link
Author

WTFox commented Jan 29, 2018

I just noticed that the quoting is a bit off. Marking this as a WIP until I fix it and push.

@WTFox WTFox changed the title Add Support for Django 1.11 and add Python 3 support. [WIP] Add Support for Django 1.11 and add Python 3 support. Jan 29, 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.

6 participants