Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed #21283 -- Added support for migrations if models is a package. #1772

Closed
wants to merge 1 commit into from

3 participants

@loic
Collaborator

Refs #14300.

django/db/models/loading.py
@@ -185,6 +185,16 @@ def get_apps(self):
return [elt[0] for elt in apps]
+ def _get_app_package(self, app):
+ if hasattr(app, '__path__'): # models/__init__.py package
+ app_package = '.'.join(app.__package__.split('.')[:-1])
+ else: # models.py module
+ app_package = app.__package__
+ return app_package
+
+ def get_app_package(self, app_label):
@MarkusH Collaborator
MarkusH added a note

I actually don't see any need for this function. Have a look at https://github.com/django/django/pull/1763/files#diff-7d0e679c6018bf032defd15cf12a2520L71

@loic Collaborator
loic added a note

As discussed on IRC, it's not strictly necessary but it's good to have a canonical way of doing this in Django. That way new features don't have to reinvent the wheel, and don't accidentally forget to handle model packages in the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/migrations/models/__init__.py
@@ -0,0 +1,2 @@
+from .unicode import *
@timgraham Owner

I'm trying to eradicate import * and use __all__ so we don't have unused imports (see #1770). Let me know if you have thoughts on the matter.

@loic Collaborator
loic added a note

Fixed it since * is not needed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/migrations/models/__init__.py
@@ -0,0 +1,2 @@
+from .unicode import UnicodeModel
+from .unserializable import UnserializableModel
@timgraham Owner

__all__ = (UnicodeModel, UnserializableModel) ?

@loic Collaborator
loic added a note

Indeed, I missed the point, the issue wasn't the * but the resulting unused import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Owner

My mistake regarding my comment earlier about it working for me before your patch was applied, I didn't read the ticket carefully. This indeed works for me. With this change, are we now testing the code path where models is not a package though?

@loic
Collaborator

@timgraham, the code path only differs on that else clause: https://github.com/loic/django/blob/ebcfecd371d9d7603f7353d00522faef414b6688/django/db/models/loading.py#L191.

If we want to test both code paths, it warrants an entirely new app, with it's own entry in ALWAYS_INSTALLED_APPS; dunno if it's necessary. Otherwise, I may be able to fake it, by testing only the output of MigrationWriter.path.

@loic loic Fixed #21283 -- Added support for migrations if models is a package.
Refs #14300.

Thanks Markus Holtermann for the report and the original attempt.
509ca76
@timgraham
Owner

merged in 5841104 - thanks!

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 19, 2013
  1. @loic

    Fixed #21283 -- Added support for migrations if models is a package.

    loic authored
    Refs #14300.
    
    Thanks Markus Holtermann for the report and the original attempt.
This page is out of date. Refresh to see the latest.
View
4 django/db/migrations/loader.py
@@ -41,8 +41,8 @@ def __init__(self, connection):
def migrations_module(cls, app_label):
if app_label in settings.MIGRATION_MODULES:
return settings.MIGRATION_MODULES[app_label]
- app = cache.get_app(app_label)
- return ".".join(app.__name__.split(".")[:-1] + ["migrations"])
+ else:
+ return '%s.migrations' % cache.get_app_package(app_label)
def load_disk(self):
"""
View
18 django/db/migrations/writer.py
@@ -61,20 +61,22 @@ def filename(self):
@property
def path(self):
- migrations_module_name = MigrationLoader.migrations_module(self.migration.app_label)
- app_module = cache.get_app(self.migration.app_label)
+ migrations_package_name = MigrationLoader.migrations_module(self.migration.app_label)
# See if we can import the migrations module directly
try:
- migrations_module = import_module(migrations_module_name)
+ migrations_module = import_module(migrations_package_name)
basedir = os.path.dirname(migrations_module.__file__)
except ImportError:
+ app = cache.get_app(self.migration.app_label)
+ app_path = cache._get_app_path(app)
+ app_package_name = cache._get_app_package(app)
+ migrations_package_basename = migrations_package_name.split(".")[-1]
+
# Alright, see if it's a direct submodule of the app
- oneup = ".".join(migrations_module_name.split(".")[:-1])
- app_oneup = ".".join(app_module.__name__.split(".")[:-1])
- if oneup == app_oneup:
- basedir = os.path.join(os.path.dirname(app_module.__file__), migrations_module_name.split(".")[-1])
+ if '%s.%s' % (app_package_name, migrations_package_basename) == migrations_package_name:
+ basedir = os.path.join(app_path, migrations_package_basename)
else:
- raise ImportError("Cannot open migrations module %s for app %s" % (migrations_module_name, self.migration.app_label))
+ raise ImportError("Cannot open migrations module %s for app %s" % (migrations_package_name, self.migration.app_label))
return os.path.join(basedir, self.filename)
@classmethod
View
7 django/db/models/loading.py
@@ -185,6 +185,12 @@ def get_apps(self):
return [elt[0] for elt in apps]
+ def _get_app_package(self, app):
+ return '.'.join(app.__name__.split('.')[:-1])
+
+ def get_app_package(self, app_label):
+ return self._get_app_package(self.get_app(app_label))
+
def _get_app_path(self, app):
if hasattr(app, '__path__'): # models/__init__.py package
app_path = app.__path__[0]
@@ -380,6 +386,7 @@ def __init__(self):
# These methods were always module level, so are kept that way for backwards
# compatibility.
get_apps = cache.get_apps
+get_app_package = cache.get_app_package
get_app_path = cache.get_app_path
get_app_paths = cache.get_app_paths
get_app = cache.get_app
View
0  tests/migrations/migrations_test_apps/__init__.py
No changes.
View
0  tests/migrations/migrations_test_apps/normal/__init__.py
No changes.
View
0  tests/migrations/migrations_test_apps/normal/models.py
No changes.
View
0  tests/migrations/migrations_test_apps/with_package_model/__init__.py
No changes.
View
0  tests/migrations/migrations_test_apps/with_package_model/models/__init__.py
No changes.
View
28 tests/migrations/test_writer.py
@@ -2,12 +2,15 @@
from __future__ import unicode_literals
+import copy
import datetime
+import os
from django.utils import six
-from django.test import TestCase
-from django.db.migrations.writer import MigrationWriter
+from django.test import TestCase, override_settings
from django.db import models, migrations
+from django.db.migrations.writer import MigrationWriter
+from django.db.models.loading import cache
from django.utils.translation import ugettext_lazy as _
@@ -95,3 +98,24 @@ def test_simple_migration(self):
# Just make sure it runs for now, and that things look alright.
result = self.safe_exec(output)
self.assertIn("Migration", result)
+
+ def test_migration_path(self):
+ _old_app_store = copy.deepcopy(cache.app_store)
+
+ test_apps = [
+ 'migrations.migrations_test_apps.normal',
+ 'migrations.migrations_test_apps.with_package_model',
+ ]
+
+ base_dir = os.path.dirname(os.path.dirname(__file__))
+
+ try:
+ with override_settings(INSTALLED_APPS=test_apps):
+ for app in test_apps:
+ cache.load_app(app)
+ migration = migrations.Migration('0001_initial', app.split('.')[-1])
+ expected_path = os.path.join(base_dir, *(app.split('.') + ['migrations', '0001_initial.py']))
+ writer = MigrationWriter(migration)
+ self.assertEqual(writer.path, expected_path)
+ finally:
+ cache.app_store = _old_app_store
Something went wrong with that request. Please try again.