From 2cae6334be880077cd6d4a8a744618ab828045ac Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Tue, 23 Jun 2020 17:44:15 -0700 Subject: [PATCH 01/12] Rely on django for module name resolution --- pylint_django/transforms/foreignkey.py | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 849a2938..c396ede6 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -87,10 +87,28 @@ def infer_key_classes(node, context=None): module_name = current_module.name elif not module_name.endswith('models'): # otherwise Django allows specifying an app name first, e.g. - # ForeignKey('auth.User') so we try to convert that to - # 'auth.models', 'User' which works nicely with the `endswith()` - # comparison below - module_name += '.models' + # ForeignKey('auth.User') + + supported_django_version_installed = False + try: + import django + supported_django_version_installed = (django.VERSION[0] == 3) + except ImportError: + pass + + if supported_django_version_installed: + # If Django is installed we can use it to resolve the module name + from django.apps import apps + + app = app.get_app_config(module_name) + model = app.get_model(model_name) + module_name = model.__module__ + else: + # Otherwise we try to convert that to + # 'auth.models', 'User' which works nicely with the `endswith()` + # comparison below + module_name += '.models' + # ensure that module is loaded in astroid_cache, for cases when models is a package if module_name not in MANAGER.astroid_cache: MANAGER.ast_from_module_name(module_name) From 9575853c73aa951e6a7f3c42af88f06ba30aeb08 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 10:46:55 -0700 Subject: [PATCH 02/12] Remove version check --- pylint_django/transforms/foreignkey.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index c396ede6..4a551496 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -89,14 +89,14 @@ def infer_key_classes(node, context=None): # otherwise Django allows specifying an app name first, e.g. # ForeignKey('auth.User') - supported_django_version_installed = False + django_installed = False try: import django - supported_django_version_installed = (django.VERSION[0] == 3) + django_installed = True except ImportError: pass - if supported_django_version_installed: + if django_installed: # If Django is installed we can use it to resolve the module name from django.apps import apps From 8dd8abb55eaa5ed06d5c1b327cff9e723f3271fb Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 11:44:50 -0700 Subject: [PATCH 03/12] Fix typo --- pylint_django/transforms/foreignkey.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 4a551496..36f98c37 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -100,7 +100,7 @@ def infer_key_classes(node, context=None): # If Django is installed we can use it to resolve the module name from django.apps import apps - app = app.get_app_config(module_name) + app = apps.get_app_config(module_name) model = app.get_model(model_name) module_name = model.__module__ else: From 023d412700a27f2ca7702fe1315ce342a74a6aff Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 11:48:14 -0700 Subject: [PATCH 04/12] Add test case --- pylint_django/tests/conftest.py | 12 ++++++++++++ pylint_django/tests/input/func_string_foreignkey.py | 10 ++++++++++ pylint_django/tests/test_app/__init__.py | 0 pylint_django/tests/test_app/apps.py | 5 +++++ pylint_django/tests/test_app/models.py | 5 +++++ pylint_django/transforms/foreignkey.py | 1 + 6 files changed, 33 insertions(+) create mode 100644 pylint_django/tests/conftest.py create mode 100644 pylint_django/tests/input/func_string_foreignkey.py create mode 100644 pylint_django/tests/test_app/__init__.py create mode 100644 pylint_django/tests/test_app/apps.py create mode 100644 pylint_django/tests/test_app/models.py diff --git a/pylint_django/tests/conftest.py b/pylint_django/tests/conftest.py new file mode 100644 index 00000000..118990dd --- /dev/null +++ b/pylint_django/tests/conftest.py @@ -0,0 +1,12 @@ +import os + +# Build paths inside the project like this: os.path.join(BASE_DIR, ...) +BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + +SECRET_KEY = 'fake-key' + +DEBUG = True + +INSTALLED_APPS = [ + 'test_app', +] diff --git a/pylint_django/tests/input/func_string_foreignkey.py b/pylint_django/tests/input/func_string_foreignkey.py new file mode 100644 index 00000000..b13d160a --- /dev/null +++ b/pylint_django/tests/input/func_string_foreignkey.py @@ -0,0 +1,10 @@ +""" +Checks that PyLint correctly handles string foreign keys +https://github.com/PyCQA/pylint-django/issues/243 +""" +# pylint: disable=missing-docstring +from django.db import models + + +class Book(models.Model): + author = models.ForeignKey("test_app.Author", models.CASCADE) diff --git a/pylint_django/tests/test_app/__init__.py b/pylint_django/tests/test_app/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/pylint_django/tests/test_app/apps.py b/pylint_django/tests/test_app/apps.py new file mode 100644 index 00000000..fc04070e --- /dev/null +++ b/pylint_django/tests/test_app/apps.py @@ -0,0 +1,5 @@ +from django.apps import AppConfig + + +class TestAppConfig(AppConfig): + name = 'test_app' diff --git a/pylint_django/tests/test_app/models.py b/pylint_django/tests/test_app/models.py new file mode 100644 index 00000000..288898ac --- /dev/null +++ b/pylint_django/tests/test_app/models.py @@ -0,0 +1,5 @@ +from django.db import models + + +class Author(models.Model): + pass diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 36f98c37..a0c317bc 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -98,6 +98,7 @@ def infer_key_classes(node, context=None): if django_installed: # If Django is installed we can use it to resolve the module name + django.setup() from django.apps import apps app = apps.get_app_config(module_name) From 9054dfbcd89c01312c2ca6efa3bf22bcb7536a7e Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 11:58:04 -0700 Subject: [PATCH 05/12] Set DJANGO_SETTINGS_MODULE if not set for tests --- pylint_django/tests/test_func.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pylint_django/tests/test_func.py b/pylint_django/tests/test_func.py index b0e8fc84..e758ed5c 100644 --- a/pylint_django/tests/test_func.py +++ b/pylint_django/tests/test_func.py @@ -6,6 +6,8 @@ import pylint +os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'pylint_django.tests.conftest') + try: # pylint 2.5: test_functional has been moved to pylint.testutils from pylint.testutils import FunctionalTestFile, LintModuleTest From 5e67fd7abe681b29662cc0ef3679e917f1c0b88e Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 12:05:29 -0700 Subject: [PATCH 06/12] Fix failing tests Fallback to previous behavior if model cannot be resolved by Django. --- pylint_django/transforms/foreignkey.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index a0c317bc..6d5b45e0 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -89,22 +89,26 @@ def infer_key_classes(node, context=None): # otherwise Django allows specifying an app name first, e.g. # ForeignKey('auth.User') - django_installed = False + use_django_model_resolving = False try: import django - django_installed = True + use_django_model_resolving = True except ImportError: pass - if django_installed: + if use_django_model_resolving: # If Django is installed we can use it to resolve the module name - django.setup() - from django.apps import apps + try: + django.setup() + from django.apps import apps - app = apps.get_app_config(module_name) - model = app.get_model(model_name) - module_name = model.__module__ - else: + app = apps.get_app_config(module_name) + model = app.get_model(model_name) + module_name = model.__module__ + except LookupError: + use_django_model_resolving = False + + if not use_django_model_resolving: # Otherwise we try to convert that to # 'auth.models', 'User' which works nicely with the `endswith()` # comparison below From 88b43fa41cc48b8421acaf87b818e4ed365fe9d4 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 12:34:20 -0700 Subject: [PATCH 07/12] Code cleanup --- pylint_django/transforms/foreignkey.py | 36 ++++++++++++++------------ 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 6d5b45e0..0affc8ee 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -1,4 +1,5 @@ from itertools import chain +from importlib.util import find_spec from astroid import ( MANAGER, nodes, InferenceError, inference_tip, @@ -41,6 +42,18 @@ def _get_model_class_defs_from_module(module, model_name, module_name): return class_defs +def _module_name_from_django_model_resolution(model_name, module_name): + import django # pylint: disable=import-outside-toplevel + django.setup() + from django.apps import apps # pylint: disable=import-outside-toplevel + + app = apps.get_app_config(module_name) + model = app.get_model(model_name) + module_name = model.__module__ + + return module_name + + def infer_key_classes(node, context=None): keyword_args = [] if node.keywords: @@ -89,31 +102,22 @@ def infer_key_classes(node, context=None): # otherwise Django allows specifying an app name first, e.g. # ForeignKey('auth.User') - use_django_model_resolving = False - try: - import django - use_django_model_resolving = True - except ImportError: - pass + django_spec = find_spec('django') + use_django_model_resolution = bool(django_spec) - if use_django_model_resolving: + if use_django_model_resolution: # If Django is installed we can use it to resolve the module name try: - django.setup() - from django.apps import apps - - app = apps.get_app_config(module_name) - model = app.get_model(model_name) - module_name = model.__module__ + module_name = _module_name_from_django_model_resolution(model_name, module_name) except LookupError: - use_django_model_resolving = False + use_django_model_resolution = False - if not use_django_model_resolving: + if not use_django_model_resolution: # Otherwise we try to convert that to # 'auth.models', 'User' which works nicely with the `endswith()` # comparison below module_name += '.models' - + # ensure that module is loaded in astroid_cache, for cases when models is a package if module_name not in MANAGER.astroid_cache: MANAGER.ast_from_module_name(module_name) From 7d11aecd16e6f5a814d71d625e614de40f5e5ee9 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Sun, 12 Jul 2020 12:44:00 -0700 Subject: [PATCH 08/12] Update README --- README.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.rst b/README.rst index 684ca828..6e6ffa6d 100644 --- a/README.rst +++ b/README.rst @@ -95,9 +95,10 @@ This plugin is disabled by default! To enable it:: Known issues ------------ -If you reference foreign-key models by their name (as string) ``pylint-django`` may not be -able to find the model and will report issues because it has no idea what the underlying -type of this field is. Supported options are:: +If Django is not installed and you reference foreign-key models by their name (as +string) ``pylint-django`` may not be able to find the model and will report issues +because it has no idea what the underlying type of this field is. Supported options +are:: - ``self`` and ``Model`` - look for this class in the current module which is being examined - ``app.Model`` - try loading ``app.models`` into the AST parser and look for ``Model`` there From 88a9bb0c62f8844e97e03e5edb206442062625c0 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Wed, 15 Jul 2020 07:50:48 -0700 Subject: [PATCH 09/12] atodorov's comments --- README.rst | 7 +++---- pylint_django/tests/conftest.py | 1 + .../tests/{ => input}/test_app/__init__.py | 0 pylint_django/tests/{ => input}/test_app/apps.py | 0 .../tests/{ => input}/test_app/models.py | 0 pylint_django/transforms/foreignkey.py | 16 ++++++---------- 6 files changed, 10 insertions(+), 14 deletions(-) rename pylint_django/tests/{ => input}/test_app/__init__.py (100%) rename pylint_django/tests/{ => input}/test_app/apps.py (100%) rename pylint_django/tests/{ => input}/test_app/models.py (100%) diff --git a/README.rst b/README.rst index 6e6ffa6d..684ca828 100644 --- a/README.rst +++ b/README.rst @@ -95,10 +95,9 @@ This plugin is disabled by default! To enable it:: Known issues ------------ -If Django is not installed and you reference foreign-key models by their name (as -string) ``pylint-django`` may not be able to find the model and will report issues -because it has no idea what the underlying type of this field is. Supported options -are:: +If you reference foreign-key models by their name (as string) ``pylint-django`` may not be +able to find the model and will report issues because it has no idea what the underlying +type of this field is. Supported options are:: - ``self`` and ``Model`` - look for this class in the current module which is being examined - ``app.Model`` - try loading ``app.models`` into the AST parser and look for ``Model`` there diff --git a/pylint_django/tests/conftest.py b/pylint_django/tests/conftest.py index 118990dd..5438192e 100644 --- a/pylint_django/tests/conftest.py +++ b/pylint_django/tests/conftest.py @@ -10,3 +10,4 @@ INSTALLED_APPS = [ 'test_app', ] + diff --git a/pylint_django/tests/test_app/__init__.py b/pylint_django/tests/input/test_app/__init__.py similarity index 100% rename from pylint_django/tests/test_app/__init__.py rename to pylint_django/tests/input/test_app/__init__.py diff --git a/pylint_django/tests/test_app/apps.py b/pylint_django/tests/input/test_app/apps.py similarity index 100% rename from pylint_django/tests/test_app/apps.py rename to pylint_django/tests/input/test_app/apps.py diff --git a/pylint_django/tests/test_app/models.py b/pylint_django/tests/input/test_app/models.py similarity index 100% rename from pylint_django/tests/test_app/models.py rename to pylint_django/tests/input/test_app/models.py diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 0affc8ee..32a4ae72 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -101,18 +101,14 @@ def infer_key_classes(node, context=None): elif not module_name.endswith('models'): # otherwise Django allows specifying an app name first, e.g. # ForeignKey('auth.User') + django_model_resolution_failed = False - django_spec = find_spec('django') - use_django_model_resolution = bool(django_spec) + try: + module_name = _module_name_from_django_model_resolution(model_name, module_name) + except LookupError: + django_model_resolution_failed = True - if use_django_model_resolution: - # If Django is installed we can use it to resolve the module name - try: - module_name = _module_name_from_django_model_resolution(model_name, module_name) - except LookupError: - use_django_model_resolution = False - - if not use_django_model_resolution: + if django_model_resolution_failed: # Otherwise we try to convert that to # 'auth.models', 'User' which works nicely with the `endswith()` # comparison below From c664bf8c195a16dc91b4705aacc4f4a6ef919f2c Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Wed, 15 Jul 2020 22:37:18 -0700 Subject: [PATCH 10/12] Cleanup --- pylint_django/tests/conftest.py | 1 - ...ng_foreignkey.py => func_noerror_string_foreignkey.py} | 0 pylint_django/tests/input/models/author.py | 3 ++- pylint_django/tests/input/test_app/models.py | 6 +----- pylint_django/transforms/foreignkey.py | 8 +------- 5 files changed, 4 insertions(+), 14 deletions(-) rename pylint_django/tests/input/{func_string_foreignkey.py => func_noerror_string_foreignkey.py} (100%) diff --git a/pylint_django/tests/conftest.py b/pylint_django/tests/conftest.py index 5438192e..118990dd 100644 --- a/pylint_django/tests/conftest.py +++ b/pylint_django/tests/conftest.py @@ -10,4 +10,3 @@ INSTALLED_APPS = [ 'test_app', ] - diff --git a/pylint_django/tests/input/func_string_foreignkey.py b/pylint_django/tests/input/func_noerror_string_foreignkey.py similarity index 100% rename from pylint_django/tests/input/func_string_foreignkey.py rename to pylint_django/tests/input/func_noerror_string_foreignkey.py diff --git a/pylint_django/tests/input/models/author.py b/pylint_django/tests/input/models/author.py index 93b9fb90..e09af25b 100644 --- a/pylint_django/tests/input/models/author.py +++ b/pylint_django/tests/input/models/author.py @@ -3,4 +3,5 @@ class Author(models.Model): - pass + class Meta: + app_label = 'test_app' diff --git a/pylint_django/tests/input/test_app/models.py b/pylint_django/tests/input/test_app/models.py index 288898ac..4062f6e0 100644 --- a/pylint_django/tests/input/test_app/models.py +++ b/pylint_django/tests/input/test_app/models.py @@ -1,5 +1 @@ -from django.db import models - - -class Author(models.Model): - pass +from models.author import Author # noqa: F401 diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 32a4ae72..8fad8ba9 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -1,5 +1,4 @@ from itertools import chain -from importlib.util import find_spec from astroid import ( MANAGER, nodes, InferenceError, inference_tip, @@ -101,15 +100,10 @@ def infer_key_classes(node, context=None): elif not module_name.endswith('models'): # otherwise Django allows specifying an app name first, e.g. # ForeignKey('auth.User') - django_model_resolution_failed = False - try: module_name = _module_name_from_django_model_resolution(model_name, module_name) except LookupError: - django_model_resolution_failed = True - - if django_model_resolution_failed: - # Otherwise we try to convert that to + # If Django's model resolution fails we try to convert that to # 'auth.models', 'User' which works nicely with the `endswith()` # comparison below module_name += '.models' From 19e8cea9561300563ee8f6a1a82ed6047bf096f0 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Mon, 20 Jul 2020 21:38:05 -0700 Subject: [PATCH 11/12] Address atodorov's comments --- pylint_django/tests/conftest.py | 12 ------------ .../tests/input/func_noerror_string_foreignkey.py | 1 + pylint_django/tests/settings.py | 12 ++++++++++++ pylint_django/tests/test_func.py | 2 +- pylint_django/transforms/foreignkey.py | 8 ++++---- 5 files changed, 18 insertions(+), 17 deletions(-) delete mode 100644 pylint_django/tests/conftest.py create mode 100644 pylint_django/tests/settings.py diff --git a/pylint_django/tests/conftest.py b/pylint_django/tests/conftest.py deleted file mode 100644 index 118990dd..00000000 --- a/pylint_django/tests/conftest.py +++ /dev/null @@ -1,12 +0,0 @@ -import os - -# Build paths inside the project like this: os.path.join(BASE_DIR, ...) -BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) - -SECRET_KEY = 'fake-key' - -DEBUG = True - -INSTALLED_APPS = [ - 'test_app', -] diff --git a/pylint_django/tests/input/func_noerror_string_foreignkey.py b/pylint_django/tests/input/func_noerror_string_foreignkey.py index b13d160a..22985452 100644 --- a/pylint_django/tests/input/func_noerror_string_foreignkey.py +++ b/pylint_django/tests/input/func_noerror_string_foreignkey.py @@ -8,3 +8,4 @@ class Book(models.Model): author = models.ForeignKey("test_app.Author", models.CASCADE) + user = models.ForeignKey("auth.User", models.PROTECT) diff --git a/pylint_django/tests/settings.py b/pylint_django/tests/settings.py new file mode 100644 index 00000000..eb12bc79 --- /dev/null +++ b/pylint_django/tests/settings.py @@ -0,0 +1,12 @@ +SECRET_KEY = 'fake-key' + +INSTALLED_APPS = [ + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'test_app', +] + +MIDDLEWARE = [ + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', +] diff --git a/pylint_django/tests/test_func.py b/pylint_django/tests/test_func.py index e758ed5c..ef2a03a0 100644 --- a/pylint_django/tests/test_func.py +++ b/pylint_django/tests/test_func.py @@ -6,7 +6,7 @@ import pylint -os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'pylint_django.tests.conftest') +os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'pylint_django.tests.settings') try: # pylint 2.5: test_functional has been moved to pylint.testutils diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index 8fad8ba9..fd8db55a 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -6,6 +6,9 @@ ) from astroid.nodes import ClassDef, Attribute +import django +from django.apps import apps + from pylint_django.utils import node_is_subclass @@ -42,15 +45,12 @@ def _get_model_class_defs_from_module(module, model_name, module_name): def _module_name_from_django_model_resolution(model_name, module_name): - import django # pylint: disable=import-outside-toplevel django.setup() - from django.apps import apps # pylint: disable=import-outside-toplevel app = apps.get_app_config(module_name) model = app.get_model(model_name) - module_name = model.__module__ - return module_name + return model.__module__ def infer_key_classes(node, context=None): From 19e601b7353f0ce2e084e25685c84566b3228fc6 Mon Sep 17 00:00:00 2001 From: Alejandro Angulo Date: Mon, 20 Jul 2020 21:48:17 -0700 Subject: [PATCH 12/12] Undo making django imports top-level Making the django imports top-level imports caused the build to break (django not installed check failed). --- pylint_django/transforms/foreignkey.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index fd8db55a..d8375713 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -6,9 +6,6 @@ ) from astroid.nodes import ClassDef, Attribute -import django -from django.apps import apps - from pylint_django.utils import node_is_subclass @@ -45,7 +42,9 @@ def _get_model_class_defs_from_module(module, model_name, module_name): def _module_name_from_django_model_resolution(model_name, module_name): + import django # pylint: disable=import-outside-toplevel django.setup() + from django.apps import apps # pylint: disable=import-outside-toplevel app = apps.get_app_config(module_name) model = app.get_model(model_name)