Improved Django settings handling #287
Conversation
…he `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)
…king - 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
…nary 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)
Pull Request Test Coverage Report for Build 1233
💛 - Coveralls |
carlio
force-pushed
the
feature/improve-django-settings-handling
branch
from
a5ff57b
to
cc77809
Compare
…gs and the reason for doing so
carlio
force-pushed
the
feature/improve-django-settings-handling
branch
from
cc77809
to
f7b556b
Compare
| @@ -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 | |||
There was a problem hiding this comment.
+1 for adding the TODOs as separate issues so we don't forget about them. Interestingly enough the pylint test job in Travis should be catching this.
|
|
||
|
|
||
| foreignkey.add_transform(astroid.MANAGER) |
There was a problem hiding this comment.
Could benefit from a comment either here or on the doc-string text and/or the foreignkey module to explicitly state why foreignkey.add_transform() isn't called like for the other modules.
There was a problem hiding this comment.
There is a module-level docstring explaining this already which I think is sufficient: https://github.com/PyCQA/pylint-django/blob/feature/improve-django-settings-handling/pylint_django/transforms/__init__.py#L1
There was a problem hiding this comment.
I also added this issue as a discussion for moving all transforms/augmentations into a checker rather than being loaded immediately by the plugin: #290
| @@ -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] | |||
There was a problem hiding this comment.
The reason for this line & the reason why it is failing is the same - it tries installing pylint-django and all of its dependencies from tartballs. One of them is either new or stopped shipping tarballs, and only has wheel packages. See 10 lines below for how we deal with another package which doesn't ship wheels.
My preferred way for dealing with this is to find which package is the one not providing tarballs and
- Open an issue against it
- Install it explicitly by itself.
There was a problem hiding this comment.
For now I'll accept this in our own CI while I track down which dependency is the guilty party - I am fairly sure it's clikit and I'll raise a PR/issue there if that's true or as you suggest, explicitly install it ourselves.
There was a problem hiding this comment.
I created a new issue to separate this out: #291
This revisits #277 and #243 @atodorov @alejandro-angulo
I had a new attempt atdealing with django settings required by the foreign key string handling and came up with this solution:
DJANGO_SETTINGS_MODULEis specified, those settings will be used.pylint --load-plugins=pylint_django --django-settings-module=myproject.settingswhich I assume would work from a.pylintrctoo.E5110) instructing the user to configure Django manually to improve linting. I set this as ERROR because it's more severe than just a warning since it is kind of an error in the testing (not in the code being tested, but the testing CI setup). It is not fatal, however, as it can continue to run.This is done by moving the Foreign Key checks into a checker rather than loading the transforms in when the plugin is loaded. This allows for using the
open()"setup" method to do any pre-check startup, in this case, running the Djangosettings.configure()method manually rather than allowing Django to do it by itself if it has an environment variable.Using a checker also means that a commandline option can be used as an additional option for configuring Django settings, which will allow it to be used in pre-commit for example (thus fixing #286)
Note: this also had to remove the
--no-binary :all:from thescripts/build.shsanity check because it seems that a newer release of isort (which pylint depends on) requires clikit and when--no-binaryis used, pylint-django fails to install. I'm not sure of the consequences of this but I assume that if pylint needs it to install, we have to accept that. This is not related to the main contents of the PR, just something I had to do to get the latest versions to pass CI.