12288 Added new check on INSTALL_APPS for unique values #1445

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@ArcTanSusan

https://code.djangoproject.com/ticket/12288 Removing lines 110-111 in django/django/conf/init.py causes the test to fail, but putting in the lines 110-111 allow tests to succeed. I've confirmed that the test works. In addition, the full Django test suite all passed. Thanks @timgraham @jdunck @charettes for the advice and code review.

I'll appreciate feedback; feel free to code review. Thanks.

@timgraham timgraham commented on an outdated diff Aug 8, 2013
tests/settings_tests/tests.py
+ Applications in INSTALLED_APPS must be unique.
+ """
+ settings_module = settings
+
+ def setUp(self):
+ self._installed_apps = self.settings_module.INSTALLED_APPS
+
+ def tearDown(self):
+ self.settings_module.INSTALLED_APPS = self._installed_apps
+
+ def test_unique(self):
+ self.settings_module.INSTALLED_APPS = ("myApp1","myApp2")
+ myAppsTest1 = ("myApp1","myApp1","myApp2")
+ self.assertEqual(len(set(myAppsTest1)), len(self.settings_module.INSTALLED_APPS))
+
+ self.settings_module.INSTALLED_APPS = ("myApp1","myApp2","myApp3")
@timgraham
timgraham Aug 8, 2013 Django member

This second assertion isn't what you want (It'll pass before and after the code change I believe.) Instead, you should be setting INSTALLED_APPS to a tuple with duplicates and using with self.assertRaises(ImproperlyConfigured)

@timgraham timgraham commented on an outdated diff Aug 8, 2013
tests/settings_tests/tests.py
+class UniqueSettngsTests(TestCase):
+ """
+ Tests for the INSTALLED_APPS setting.
+
+ Applications in INSTALLED_APPS must be unique.
+ """
+ settings_module = settings
+
+ def setUp(self):
+ self._installed_apps = self.settings_module.INSTALLED_APPS
+
+ def tearDown(self):
+ self.settings_module.INSTALLED_APPS = self._installed_apps
+
+ def test_unique(self):
+ self.settings_module.INSTALLED_APPS = ("myApp1","myApp2")
@timgraham
timgraham Aug 8, 2013 Django member

There should be a space after each comma when defining tuples

@timgraham timgraham commented on an outdated diff Aug 8, 2013
django/conf/__init__.py
@@ -107,6 +107,10 @@ def __setattr__(self, name, value):
elif name == "ALLOWED_INCLUDE_ROOTS" and isinstance(value, six.string_types):
raise ValueError("The ALLOWED_INCLUDE_ROOTS setting must be set "
"to a tuple, not a string.")
+ elif name == "INSTALLED_APPS" and len(value) != len(set(value)):
+ raise ImproperlyConfigured("The %s setting must contain unique values. "
+ "Please fix your settings." % name)
@timgraham
timgraham Aug 8, 2013 Django member

I think we can omit "Please fix your settings."

@timgraham timgraham commented on an outdated diff Aug 8, 2013
django/conf/__init__.py
@@ -107,6 +107,10 @@ def __setattr__(self, name, value):
elif name == "ALLOWED_INCLUDE_ROOTS" and isinstance(value, six.string_types):
raise ValueError("The ALLOWED_INCLUDE_ROOTS setting must be set "
"to a tuple, not a string.")
+ elif name == "INSTALLED_APPS" and len(value) != len(set(value)):
+ raise ImproperlyConfigured("The %s setting must contain unique values. "
@timgraham
timgraham Aug 8, 2013 Django member

no need to use string interpolation here "The INSTALLED_APPS settings..."

@timgraham
Member

Looks good, merged in 2ac8901 - thanks Susan!

@timgraham timgraham closed this Aug 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment