Merge pull request #134 from treyhunner/code-quality

Some cleanup

runtests.py

@@ -52,7 +52,7 @@ def main():
         django.setup()
     try:
         from django.test.runner import DiscoverRunner
-    except:
+    except ImportError:
         from django.test.simple import DjangoTestSuiteRunner
         failures = DjangoTestSuiteRunner(failfast=False).run_tests(['tests'])
     else:

simple_history/__init__.py

@@ -15,7 +15,7 @@ def register(model, app=None, manager_name='history', **records_config):
     `HistoricalManager` instance directly to `model`.
     """
     from . import models
-    if not model._meta.db_table in models.registered_models:
+    if model._meta.db_table not in models.registered_models:
         records = models.HistoricalRecords(**records_config)
         records.manager_name = manager_name
         records.module = app and ("%s.models" % app) or model.__module__

simple_history/admin.py

@@ -1,10 +1,7 @@
 from __future__ import unicode_literals
 
 from django.core.exceptions import PermissionDenied
-try:
-    from django.conf.urls import patterns, url
-except ImportError:
-    from django.conf.urls.defaults import patterns, url
+from django.conf.urls import patterns, url
 from django.contrib import admin
 from django.contrib.admin import helpers
 from django.contrib.contenttypes.models import ContentType
@@ -78,18 +75,14 @@ def history_view(self, request, object_id, extra_context=None):
                       dictionary=context, current_app=self.admin_site.name)
 
     def history_form_view(self, request, object_id, version_id):
-        original_model = self.model
-        original_opts = original_model._meta
-        history = getattr(self.model,
-                          self.model._meta.simple_history_manager_attribute)
-        model = history.model
-        opts = model._meta
-        pk_name = original_opts.pk.attname
-        record = get_object_or_404(model, **{
-            pk_name: object_id,
+        original_opts = self.model._meta
+        model = getattr(
+            self.model,
+            self.model._meta.simple_history_manager_attribute).model
+        obj = get_object_or_404(model, **{
+            original_opts.pk.attname: object_id,
             'history_id': version_id,
-        })
-        obj = record.instance
+        }).instance
         obj._state.adding = False
 
         if not self.has_change_permission(request, obj):
@@ -100,19 +93,13 @@ def history_form_view(self, request, object_id, version_id):
         if request.method == 'POST':
             form = form_class(request.POST, request.FILES, instance=obj)
             if form.is_valid():
-                form_validated = True
                 new_object = self.save_form(request, form, change=True)
-            else:
-                form_validated = False
-                new_object = obj
-
-            if form_validated:
                 self.save_model(request, new_object, form, change=True)
                 form.save_m2m()
 
-                change_message = self.construct_change_message(request, form,
-                                                               formsets)
-                self.log_change(request, new_object, change_message)
+                self.log_change(request, new_object,
+                                self.construct_change_message(
+                                    request, form, formsets))
                 return self.response_change(request, new_object)
 
         else:
@@ -125,23 +112,21 @@ def history_form_view(self, request, object_id, version_id):
             self.get_readonly_fields(request, obj),
             model_admin=self,
         )
-        media = self.media + admin_form.media
 
         try:
-            model_name = original_opts.module_name
-        except AttributeError:
             model_name = original_opts.model_name
+        except AttributeError:
+            model_name = original_opts.module_name
         url_triplet = self.admin_site.name, original_opts.app_label, model_name
-        content_type_id = ContentType.objects.get_for_model(self.model).id
         context = {
             'title': _('Revert %s') % force_text(obj),
             'adminform': admin_form,
             'object_id': object_id,
             'original': obj,
             'is_popup': False,
-            'media': mark_safe(media),
+            'media': mark_safe(self.media + admin_form.media),
             'errors': helpers.AdminErrorList(form, formsets),
-            'app_label': opts.app_label,
+            'app_label': model._meta.app_label,
             'original_opts': original_opts,
             'changelist_url': reverse('%s:%s_%s_changelist' % url_triplet),
             'change_url': reverse('%s:%s_%s_change' % url_triplet,
@@ -157,8 +142,8 @@ def history_form_view(self, request, object_id, version_id):
             'has_file_field': True,
             'has_absolute_url': False,
             'form_url': '',
-            'opts': opts,
-            'content_type_id': content_type_id,
+            'opts': model._meta,
+            'content_type_id': ContentType.objects.get_for_model(self.model).id,
             'save_as': self.save_as,
             'save_on_top': self.save_on_top,
             'root_path': getattr(self.admin_site, 'root_path', None),

simple_history/manager.py

@@ -31,10 +31,10 @@ def get_queryset(self):
             return qs
 
         if isinstance(self.instance._meta.pk, models.OneToOneField):
-            filter = {self.instance._meta.pk.name + "_id": self.instance.pk}
+            key_name = self.instance._meta.pk.name + "_id"
         else:
-            filter = {self.instance._meta.pk.name: self.instance.pk}
-        return self.get_super_queryset().filter(**filter)
+            key_name = self.instance._meta.pk.name
+        return self.get_super_queryset().filter(**{key_name: self.instance.pk})
 
     get_query_set = get_queryset
 
@@ -53,7 +53,7 @@ def most_recent(self):
                 tmp.append(field.name)
         fields = tuple(tmp)
         try:
-            values = self.values_list(*fields)[0]
+            values = self.get_queryset().values_list(*fields)[0]
         except IndexError:
             raise self.instance.DoesNotExist("%s has no historical record." %
                                              self.instance._meta.object_name)
@@ -68,7 +68,7 @@ def as_of(self, date):
         """
         if not self.instance:
             return self._as_of_set(date)
-        queryset = self.filter(history_date__lte=date)
+        queryset = self.get_queryset().filter(history_date__lte=date)
         try:
             history_obj = queryset[0]
         except IndexError:
@@ -84,7 +84,7 @@ def as_of(self, date):
     def _as_of_set(self, date):
         model = type(self.model().instance)  # a bit of a hack to get the model
         pk_attr = model._meta.pk.name
-        queryset = self.filter(history_date__lte=date)
+        queryset = self.get_queryset().filter(history_date__lte=date)
         for original_pk in set(
                 queryset.order_by().values_list(pk_attr, flat=True)):
             changes = queryset.filter(**{pk_attr: original_pk})

simple_history/models.py

@@ -7,6 +7,7 @@
 except ImportError:
     apps = None
 from django.db import models, router
+from django.db.models import loading
 from django.db.models.fields.related import RelatedField
 from django.db.models.related import RelatedObject
 from django.conf import settings
@@ -99,7 +100,7 @@ def create_history_model(self, model):
         elif app_module != self.module:
             if apps is None:
                 # has meta options with app_label
-                app = models.get_app(model._meta.app_label)
+                app = loading.get_app(model._meta.app_label)
                 attrs['__module__'] = app.__name__  # full dotted name
             else:
                 # Abuse an internal API because the app registry is loading.
@@ -200,14 +201,14 @@ def post_save(self, instance, created, **kwargs):
     def post_delete(self, instance, **kwargs):
         self.create_historical_record(instance, '-')
 
-    def create_historical_record(self, instance, type):
+    def create_historical_record(self, instance, history_type):
         history_date = getattr(instance, '_history_date', now())
         history_user = self.get_history_user(instance)
         manager = getattr(instance, self.manager_name)
         attrs = {}
         for field in instance._meta.fields:
             attrs[field.attname] = getattr(instance, field.attname)
-        manager.create(history_date=history_date, history_type=type,
+        manager.create(history_date=history_date, history_type=history_type,
                        history_user=history_user, **attrs)
 
     def get_history_user(self, instance):

simple_history/tests/urls.py

@@ -1,10 +1,6 @@
 from __future__ import unicode_literals
 
-try:
-    from django.conf.urls import include, url
-except ImportError:
-    from django.conf.urls.defaults import include, url
-
+from django.conf.urls import include, url
 from django.contrib import admin
 from . import other_admin