From 36fc49fca89fcd69e599060afddaa9609bcfb520 Mon Sep 17 00:00:00 2001 From: carlio Date: Mon, 24 Aug 2020 13:33:45 +0200 Subject: [PATCH 1/8] Moved the Django foreignkey field into a separate checker to enable the `open()` pre-configuration. This allows the django settings module to be set from a command-line option or .pylintrc option as well as an environment variable (refs #286). This commit also uses default Django settings if none are specified, raising an error to the user to inform them that it is better to set it explicitly, but avoiding the need to raise a Fatal or RuntimeError if Django is not configured (refs #277 and #243) --- pylint_django/checkers/__init__.py | 2 + pylint_django/checkers/django_installed.py | 7 +- pylint_django/checkers/foreign_key_strings.py | 117 ++++++++++++++++++ pylint_django/checkers/migrations.py | 2 +- pylint_django/transforms/__init__.py | 16 ++- pylint_django/utils.py | 2 +- 6 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 pylint_django/checkers/foreign_key_strings.py diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index d036e7b5..b0a5bb00 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -4,6 +4,7 @@ from pylint_django.checkers.json_response import JsonResponseChecker from pylint_django.checkers.forms import FormChecker from pylint_django.checkers.auth_user import AuthUserChecker +from pylint_django.checkers.foreign_key_strings import ForeignKeyStringsChecker def register_checkers(linter): @@ -13,3 +14,4 @@ def register_checkers(linter): linter.register_checker(JsonResponseChecker(linter)) linter.register_checker(FormChecker(linter)) linter.register_checker(AuthUserChecker(linter)) + linter.register_checker(ForeignKeyStringsChecker(linter)) diff --git a/pylint_django/checkers/django_installed.py b/pylint_django/checkers/django_installed.py index 1a51c70c..515f47e3 100644 --- a/pylint_django/checkers/django_installed.py +++ b/pylint_django/checkers/django_installed.py @@ -20,8 +20,11 @@ class DjangoInstalledChecker(BaseChecker): } @check_messages('django-not-available') - def close(self): + def open(self): try: __import__('django') except ImportError: - self.add_message('F%s01' % BASE_ID) + # mild hack: this error is added before any modules have been inspected + # so we need to set some kind of value to prevent the linter failing to it + self.linter.set_current_module('project global') + self.add_message('django-not-available') diff --git a/pylint_django/checkers/foreign_key_strings.py b/pylint_django/checkers/foreign_key_strings.py new file mode 100644 index 00000000..648bf5a4 --- /dev/null +++ b/pylint_django/checkers/foreign_key_strings.py @@ -0,0 +1,117 @@ +from __future__ import absolute_import +from pylint.checkers import BaseChecker +from pylint.interfaces import IAstroidChecker +from pylint.checkers.utils import check_messages +from pylint_django.__pkginfo__ import BASE_ID +from pylint_django.transforms import foreignkey +import astroid + + +class ForeignKeyStringsChecker(BaseChecker): + """ + Adds transforms to be able to do type inference for model ForeignKeyField + properties which use a string to name the foreign relationship. This uses + Django's model name resolution and this checker wraps the setup to ensure + Django is able to configure itself before attempting to use the lookups. + """ + + _LONG_MESSAGE = """Finding foreign-key relationships from strings in pylint-django requires configuring Django. +This can be done via the DJANGO_SETTINGS_MODULE environment variable or the pylint option django-settings-module, eg: + + `pylint --load-plugins=pylint_django --django-settings-module=myproject.settings` + +. This can also be set as an option in a .pylintrc configuration file. + +Some basic default settings were used, however this will lead to less accurate linting. +Consider passing in an explicit Django configuration file to match your project to improve accuracy.""" + + __implements__ = (IAstroidChecker,) + + name = "Django foreign keys referenced by strings" + + options = ( + ( + "django-settings-module", + { + "default": None, + "type": "string", + "metavar": "", + "help": "A module containing Django settings to be used while linting.", + }, + ), + ) + + msgs = {"E%s10" % BASE_ID: ("Django was not configured. For more information run pylint --load-plugins=pylint_django --help-msg=django-not-configured", "django-not-configured", _LONG_MESSAGE), + "F%s10" % BASE_ID: ( + 'Provided Django settings %s could not be loaded', + 'django-settings-module-not-found', + 'The provided Django settings module %s was not found on the path' + )} + + def open(self): + self._raise_warning = False + # This is a bit of a hacky workaround. pylint-django does not *require* that + # Django is configured explicitly, and will use some basic defaults in that + # case. However, as this is a WARNING not a FATAL, the error must be raised + # with an AST node - only F and R messages are scope exempt (see + # https://github.com/PyCQA/pylint/blob/master/pylint/constants.py#L24) + + # However, testing to see if Django is configured happens in `open()` + # before any modules are inspected, as Django needs to be configured with + # defaults before the foreignkey checker can work properly. At this point, + # there are no nodes. + + # Therefore, during the initialisation in `open()`, if django was configured + # using defaults by pylint-django, it cannot raise the warning yet and + # must wait until some module is inspected to be able to raise... so that + # state is stashed in this property. + + from django.core.exceptions import ( + ImproperlyConfigured, + ) # pylint: disable=import-outside-toplevel + + try: + import django # pylint: disable=import-outside-toplevel + + django.setup() + from django.apps import apps # pylint: disable=import-outside-toplevel + except ImproperlyConfigured: + # this means that Django wasn't able to configure itself using some defaults + # provided (likely in a DJANGO_SETTINGS_MODULE environment variable) + # so see if the user has specified a pylint option + if self.config.django_settings_module is None: + # we will warn the user that they haven't actually configured Django themselves + self._raise_warning = True + # but use django defaults then... + from django.conf import settings # pylint: disable=import-outside-toplevel + settings.configure() + django.setup() + else: + # see if we can load the provided settings module + try: + from django.conf import settings, Settings # pylint: disable=import-outside-toplevel + settings.configure(Settings(self.config.django_settings_module)) + django.setup() + except ImportError: + # we could not find the provided settings module... + # at least here it is a fatal error so we can just raise this immediately + self.add_message('django-settings-module-not-found', args=self.config.django_settings_module) + # however we'll trundle on with basic settings + from django.conf import settings # pylint: disable=import-outside-toplevel + settings.configure() + django.setup() + + # now we can add the trasforms speciifc to this checker + foreignkey.add_transform(astroid.MANAGER) + + # TODO: this is a bit messy having so many inline imports but in order to avoid + # duplicating the django_installed checker, it'll do for now. In the future, merging + # those two checkers together might make sense. + + + @check_messages("django-not-configured") + def visit_module(self, node): + if self._raise_warning: + # just add it to the first node we see... which isn't nice but not sure what else to do + self.add_message("django-not-configured", node=node) + self._raise_warning = False # only raise it once... diff --git a/pylint_django/checkers/migrations.py b/pylint_django/checkers/migrations.py index 4014d412..392ed324 100644 --- a/pylint_django/checkers/migrations.py +++ b/pylint_django/checkers/migrations.py @@ -156,7 +156,7 @@ def func(apps, schema_editor): return is_migrations_module(node.parent) -def load_configuration(linter): +def load_configuration(linter): # TODO this is redundant and can be removed # don't blacklist migrations for this checker new_black_list = list(linter.config.black_list) if 'migrations' in new_black_list: diff --git a/pylint_django/transforms/__init__.py b/pylint_django/transforms/__init__.py index cf602b0f..b6b7c5aa 100644 --- a/pylint_django/transforms/__init__.py +++ b/pylint_django/transforms/__init__.py @@ -1,16 +1,24 @@ -"""Transforms.""" +""" +These transforms replace the Django types with adapted versions to provide +additional typing and method inference to pylint. All of these transforms +are considered "global" to pylint-django, in that all checks and improvements +requre them to be loaded. Additional transforms specific to checkers are loaded +by the checker rather than here. + +For example, the ForeignKeyStringsChecker loads the foreignkey.py transforms +itself as it may be disabled independently of the rest of pylint-django +""" import os import re import astroid -from pylint_django.transforms import foreignkey, fields +from pylint_django.transforms import fields - -foreignkey.add_transform(astroid.MANAGER) fields.add_transforms(astroid.MANAGER) + def _add_transform(package_name): def fake_module_builder(): """ diff --git a/pylint_django/utils.py b/pylint_django/utils.py index f91cf5aa..883ef122 100644 --- a/pylint_django/utils.py +++ b/pylint_django/utils.py @@ -8,7 +8,7 @@ from pylint_django.compat import Uninferable -PY3 = sys.version_info >= (3, 0) +PY3 = sys.version_info >= (3, 0) # TODO: pylint_django doesn't support Py2 any more def node_is_subclass(cls, *subclass_names): From 26f3af380ef356ced3890de98a8fd144f78baa2d Mon Sep 17 00:00:00 2001 From: carlio Date: Mon, 24 Aug 2020 13:54:59 +0200 Subject: [PATCH 2/8] Removing the "Django not configured" check from tox sanity/basic checking - alternative is to manually supply a basic django settings file which seems like it would clog up the repo more than one single disable flag in the tox file --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index ba833345..b5207d30 100644 --- a/tox.ini +++ b/tox.ini @@ -15,7 +15,7 @@ envlist = [testenv] commands = django_not_installed: bash -c \'pylint --load-plugins=pylint_django setup.py | grep django-not-available\' - django_is_installed: pylint --load-plugins=pylint_django setup.py + django_is_installed: pylint --load-plugins=pylint_django --disable=E5110 setup.py flake8: flake8 pylint: pylint --rcfile=tox.ini -d missing-docstring,too-many-branches,too-many-return-statements,too-many-ancestors,fixme --ignore=tests pylint_django setup readme: bash -c \'python setup.py -q sdist && twine check dist/*\' From 21487b4625d577f168351f4ad7a95569fa00b0b5 Mon Sep 17 00:00:00 2001 From: carlio Date: Mon, 24 Aug 2020 14:03:16 +0200 Subject: [PATCH 3/8] Running black against the new Foreign Key checker and fixing CHANGELOG header --- CHANGELOG.rst | 11 ++++ pylint_django/checkers/foreign_key_strings.py | 59 +++++++++++++------ pylint_django/transforms/__init__.py | 1 - pylint_django/transforms/foreignkey.py | 4 +- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 264cd962..653271b5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,6 +1,17 @@ Changelog ========= +Version 2.3.1 (TBD) +------------------- + +- Allowed configuration of the Django settings module to be used via a + commandline argument `#286 `_ +- If Django settings are not specified via a commandline argument or environment + variable, an error is issued but defaults are loaded from Django, removing the + fatal error behaviour. `#277 `__ + and `#243 `__ +- Fixed tests to work with pylint>2.6 + Version 2.3.0 (05 Aug 2020) --------------------------- diff --git a/pylint_django/checkers/foreign_key_strings.py b/pylint_django/checkers/foreign_key_strings.py index 648bf5a4..b35d03d0 100644 --- a/pylint_django/checkers/foreign_key_strings.py +++ b/pylint_django/checkers/foreign_key_strings.py @@ -1,10 +1,12 @@ from __future__ import absolute_import + +import astroid from pylint.checkers import BaseChecker -from pylint.interfaces import IAstroidChecker from pylint.checkers.utils import check_messages +from pylint.interfaces import IAstroidChecker + from pylint_django.__pkginfo__ import BASE_ID from pylint_django.transforms import foreignkey -import astroid class ForeignKeyStringsChecker(BaseChecker): @@ -41,15 +43,27 @@ class ForeignKeyStringsChecker(BaseChecker): ), ) - msgs = {"E%s10" % BASE_ID: ("Django was not configured. For more information run pylint --load-plugins=pylint_django --help-msg=django-not-configured", "django-not-configured", _LONG_MESSAGE), - "F%s10" % BASE_ID: ( - 'Provided Django settings %s could not be loaded', - 'django-settings-module-not-found', - 'The provided Django settings module %s was not found on the path' - )} + msgs = { + "E%s10" + % BASE_ID: ( + "Django was not configured. For more information run" + "pylint --load-plugins=pylint_django --help-msg=django-not-configured", + "django-not-configured", + _LONG_MESSAGE, + ), + "F%s10" + % BASE_ID: ( + "Provided Django settings %s could not be loaded", + "django-settings-module-not-found", + "The provided Django settings module %s was not found on the path", + ), + } + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._raise_warning = False def open(self): - self._raise_warning = False # This is a bit of a hacky workaround. pylint-django does not *require* that # Django is configured explicitly, and will use some basic defaults in that # case. However, as this is a WARNING not a FATAL, the error must be raised @@ -66,15 +80,15 @@ def open(self): # must wait until some module is inspected to be able to raise... so that # state is stashed in this property. - from django.core.exceptions import ( + from django.core.exceptions import ( # pylint: disable=import-outside-toplevel ImproperlyConfigured, - ) # pylint: disable=import-outside-toplevel + ) try: import django # pylint: disable=import-outside-toplevel django.setup() - from django.apps import apps # pylint: disable=import-outside-toplevel + from django.apps import apps # noqa pylint: disable=import-outside-toplevel,unused-import except ImproperlyConfigured: # this means that Django wasn't able to configure itself using some defaults # provided (likely in a DJANGO_SETTINGS_MODULE environment variable) @@ -83,21 +97,33 @@ def open(self): # we will warn the user that they haven't actually configured Django themselves self._raise_warning = True # but use django defaults then... - from django.conf import settings # pylint: disable=import-outside-toplevel + from django.conf import ( # pylint: disable=import-outside-toplevel + settings, + ) settings.configure() django.setup() else: # see if we can load the provided settings module try: - from django.conf import settings, Settings # pylint: disable=import-outside-toplevel + from django.conf import ( # pylint: disable=import-outside-toplevel + settings, + Settings, + ) + settings.configure(Settings(self.config.django_settings_module)) django.setup() except ImportError: # we could not find the provided settings module... # at least here it is a fatal error so we can just raise this immediately - self.add_message('django-settings-module-not-found', args=self.config.django_settings_module) + self.add_message( + "django-settings-module-not-found", + args=self.config.django_settings_module, + ) # however we'll trundle on with basic settings - from django.conf import settings # pylint: disable=import-outside-toplevel + from django.conf import ( # pylint: disable=import-outside-toplevel + settings, + ) + settings.configure() django.setup() @@ -108,7 +134,6 @@ def open(self): # duplicating the django_installed checker, it'll do for now. In the future, merging # those two checkers together might make sense. - @check_messages("django-not-configured") def visit_module(self, node): if self._raise_warning: diff --git a/pylint_django/transforms/__init__.py b/pylint_django/transforms/__init__.py index b6b7c5aa..03930d3d 100644 --- a/pylint_django/transforms/__init__.py +++ b/pylint_django/transforms/__init__.py @@ -18,7 +18,6 @@ fields.add_transforms(astroid.MANAGER) - def _add_transform(package_name): def fake_module_builder(): """ diff --git a/pylint_django/transforms/foreignkey.py b/pylint_django/transforms/foreignkey.py index fdc02eca..d16b47c7 100644 --- a/pylint_django/transforms/foreignkey.py +++ b/pylint_django/transforms/foreignkey.py @@ -108,10 +108,10 @@ def infer_key_classes(node, context=None): # 'auth.models', 'User' which works nicely with the `endswith()` # comparison below module_name += '.models' - except ImproperlyConfigured: + except ImproperlyConfigured as exep: raise RuntimeError("DJANGO_SETTINGS_MODULE required for resolving ForeignKey " "string references, see Usage section in README at " - "https://pypi.org/project/pylint-django/!") + "https://pypi.org/project/pylint-django/!") from exep # ensure that module is loaded in astroid_cache, for cases when models is a package if module_name not in MANAGER.astroid_cache: From c7730fabff5a3fe30d1e6b907d2bcbff8b6564b1 Mon Sep 17 00:00:00 2001 From: carlio Date: Wed, 26 Aug 2020 11:52:24 +0200 Subject: [PATCH 4/8] Pylint now depends on isort and the current version of isort has a binary dependancy on clikit, so either we have to pin isort to an earlier version or prevent checking that pylint-django can be installed with --no-binary (since, presumably, isort and therefore pylint now require binary package dependencies) --- scripts/build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/build.sh b/scripts/build.sh index 0d261070..00ba5b91 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -69,7 +69,7 @@ echo "..... Trying to install the new tarball inside a virtualenv" # note: installs with the optional dependency to verify this is still working virtualenv .venv/test-tarball source .venv/test-tarball/bin/activate -pip install --no-binary :all: -f dist/ pylint_django[with_django] +pip install -f dist/ pylint_django[with_django] pip freeze | grep Django deactivate rm -rf .venv/ From 08ad9def4a6780aa4e8e2a89bda968b136a16f93 Mon Sep 17 00:00:00 2001 From: carlio Date: Wed, 26 Aug 2020 13:00:56 +0200 Subject: [PATCH 5/8] No point sending coveralls reports for builds that do not run tests! --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 81337545..47203e1c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -45,7 +45,7 @@ script: after_success: - | - if [ -z "$SANITY_CHECK" ]; then + if [ -z "$SANITY_CHECK" ] && [ -z "$TOXENV" ]; then pip install coveralls coveralls fi From f7b556b475db4bfcee992b764987a1b0a6f5c40c Mon Sep 17 00:00:00 2001 From: carlio Date: Wed, 26 Aug 2020 13:33:09 +0200 Subject: [PATCH 6/8] Adding information to the README about hwo to configure Django settings and the reason for doing so --- README.rst | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/README.rst b/README.rst index f339aa77..8df40a61 100644 --- a/README.rst +++ b/README.rst @@ -44,10 +44,21 @@ about missing Django! Usage ----- -Ensure ``pylint-django`` is installed and on your path and then execute:: + +Ensure ``pylint-django`` is installed and on your path. In order to access some +of the internal Django features to improve pylint inspections, you should also +provide a Django settings module appropriate to your project. This can be done +either with an environment variable:: DJANGO_SETTINGS_MODULE=your.app.settings pylint --load-plugins pylint_django [..other options..] +Alternatively, this can be passed in as a commandline flag:: + + pylint --load-plugins pylint_django --django-settings-module=your.app.settings [..other options..] + +If you do not configure Django, default settings will be used but this will not include, for +example, which applications to include in `INSTALLED_APPS` and so the linting and type inference +will be less accurate. It is recommended to specify a settings module. Prospector ---------- From aa7f80b23fdec1fe635d7c8212810ab80a9c1fc8 Mon Sep 17 00:00:00 2001 From: carlio Date: Fri, 28 Aug 2020 11:18:21 +0200 Subject: [PATCH 7/8] Small typo fix --- pylint_django/checkers/foreign_key_strings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint_django/checkers/foreign_key_strings.py b/pylint_django/checkers/foreign_key_strings.py index b35d03d0..02606cce 100644 --- a/pylint_django/checkers/foreign_key_strings.py +++ b/pylint_django/checkers/foreign_key_strings.py @@ -127,7 +127,7 @@ def open(self): settings.configure() django.setup() - # now we can add the trasforms speciifc to this checker + # now we can add the trasforms specifc to this checker foreignkey.add_transform(astroid.MANAGER) # TODO: this is a bit messy having so many inline imports but in order to avoid From 7721691d73a26fea3d4c7e23b90d5cc8ad9c6f96 Mon Sep 17 00:00:00 2001 From: carlio Date: Fri, 28 Aug 2020 11:24:26 +0200 Subject: [PATCH 8/8] Changing the "module" name used when rereporting global issues --- pylint_django/checkers/django_installed.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint_django/checkers/django_installed.py b/pylint_django/checkers/django_installed.py index 515f47e3..de79e4d6 100644 --- a/pylint_django/checkers/django_installed.py +++ b/pylint_django/checkers/django_installed.py @@ -26,5 +26,5 @@ def open(self): except ImportError: # mild hack: this error is added before any modules have been inspected # so we need to set some kind of value to prevent the linter failing to it - self.linter.set_current_module('project global') + self.linter.set_current_module('pylint_django') self.add_message('django-not-available')