Add support for new load_configuration hook of pylint

[matusvalo]

This PR adds new pylint hook load_configuration(). This hook serves for adjusting configuration of linter: http://pylint.pycqa.org/en/latest/how_tos/plugins.html

Closes #181

[matusvalo]

This PR can be closed after releasing new version of pylint with pylint-dev/pylint#2644

[coveralls]

Pull Request Test Coverage Report for Build 547

• 12 of 12 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 87.697%

---

Totals
Change from base Build 534:	0.2%
Covered Lines:	613
Relevant Lines:	699

---

💛 - Coveralls

[atodorov]

How about db_performance.py checker ? It also modifes blacklist config.

Generally looks good, I'm not sure why tests failed though. The job with DJANGO=master is pulling the pylint master branch so in theory it should not fail with your changes.

[matusvalo]

@atodorov I have fixed also db_performance.py checker. Regarding tests, I don't expect them to be green. I suppose that first dependency to pylint needs to be fixed:
https://github.com/PyCQA/pylint-django/blob/bb0153ba1388e5304d5c902efa0d260883e99d1a/setup.py#L19-L22
Now, the change in pylint is just in master branch, we need to wait to release, and bump dependency of pylint to most recent version. After tests can be fixed. Or is there any other way how to proceed?

[carlio]

Perhaps I misunderstand but this would not be backwards compatible with older pylints as load_configuration would not be called by them so those extra config pieces not added? So merging this would make anything not using the newest pylint suddenly start showing pylint messages that were not there before.

I think that calling load_configuration explicitly in register would keep both functionalities for now, perhaps with a conditional based on pylint version, and certainly a comment explaining the pylint version that made the change for later compatability cleanups.

[matusvalo]

@carlio You are right. This PR needs to bump dependency to pylint version which supports load_configuration() hook. For now this hook is just in master branch and will be released sometime in future - this was also mentioned in my first comment:

This PR can be closed after releasing new version of pylint with PyCQA/pylint#2644

I am fine with adjusting this PR with backward compatibility - when older version of pylint is detected, load_configuration() will be called from register(). But this can be confusing for users since there is open issue #181 which will affect users when they have older pylint installed.

[carlio]

@matusvalo the compat.py file is exactly for things like this. The problem is the number of configurations for build systems of the type pylint==X, pylint-django so there needs to be a way make non-breaking changes more gently than rigid requirements limits on pylint.

@atodorov and I have had this discussion many times, I think he's more on your side but I certainly prefer making a smooth movement given how often a change to pylint-django can break a build. A simple 'if pylint_version < x.y.z: load_configuration()' isn't much and could be in compat.py with a good comment so it can be removed in the future.

We do need to figure out a better way to warn about things like this, perhaps a new emitted warning our regular python warn about mild version incompatabilities. Anyway I just want to avoid fixing #181 then ending up with a new issue from users with older pylints but unpinned pylint-django dependencies. That's equally confusing!

[matusvalo]

@carlio I am fine with your proposal. I will implement your comments. I think adding warning would be ideal.

[atodorov]

'if pylint_version < x.y.z: load_configuration()' isn't much and could be in compat.py with a good comment so it can be removed in the future.

+1 for that.

Re #181 - there's no way around that if people are using an older version of pylint. We'll document this when releasing the new version but really if you insist on using an older pylint there's nothing we can do about it.

I personally favor using the latest and greatest versions of dev tools but not everybody does this unfortunately.

[carlio]

All good - thanks for the compat addition, I know it's a PITA but it will save many bug reports too :-) Thanks for the PR and fixes.

[carlio]

@matusvalo oh btw don't forget to add the change to the CHANGELOG if you want. No worries it will get done for next release though 👍

[carlio]

All good - thanks for the compat addition, I know it's a PITA but it will save many bug reports too :-) Thanks for the PR and fixes.

[carlio]

wait is this the right way round? compat.REGISTER_LEGACY sounds like it should call plugin.register_legacy. Might just be a naming issue

[matusvalo]

Oh stupid mistake... I will fix it. In any case I would like to do some manual tests for this functionality + investigate whether we can print some warning when old pylint is used. Maybe some unittests can be added for this functionality what do you think?

[carlio]

It seems like it's more of a test for building against different versions, so adding something to the build matrix would be how to deal with this. Different pylint versions in the env are needed to test each branch properly. @atodorov what do you think? will the existing build matrix be enough to catch this?

[atodorov]

@carlio, @matusvalo - looking at the latest test results I see all PASS. See job 524.8 with env DJANGO=master. This is installing pylint from their master branch and the console log shows pylint version 2.3.0.

IMO we're OK on the testing side here.

[matusvalo]

Btw. I manually tried the PR and:

PR with current pylint version (2.2.2) works as expected:

$ pylint --load-plugins pylint_django foo/foo/
************* Module foo
foo/foo/__init__.py:1:0: C0102: Black listed name "foo" (blacklisted-name)

------------------------------------------------------------------
Your code has been rated at 9.60/10 (previous run: 8.80/10, +0.80)


$ pylint --load-plugins pylint_django --good-names=foo foo/foo/
************* Module foo.urls
foo/foo/urls.py:19:0: C0103: Constant name "urlpatterns" doesn't conform to UPPER_CASE naming style (invalid-name)

------------------------------------------------------------------
Your code has been rated at 9.60/10 (previous run: 9.60/10, +0.00)

BUT with with pylint from master branch I am getting the following stacktrace:

$ pylint --load-plugins pylint_django foo/foo/
Traceback (most recent call last):
  File "/home/matus/environments/pylint36/bin/pylint", line 11, in <module>
    load_entry_point('pylint==2.3.0', 'console_scripts', 'pylint')()
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint/__init__.py", line 20, in run_pylint
    Run(sys.argv[1:])
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint/lint.py", line 1537, in __init__
    linter.load_plugin_modules(self._plugins)
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint/lint.py", line 655, in load_plugin_modules
    module.register(self)
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint_django/plugin.py", line 30, in register
    apply_augmentations(linter)
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint_django/augmentations/__init__.py", line 763, in apply_augmentations
    suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_field_display_method)
  File "/home/matus/environments/pylint36/lib/python3.6/site-packages/pylint_plugin_utils/__init__.py", line 125, in suppress_message
    get_message_definition = msgs_store.get_message_definition
AttributeError: 'MessagesStore' object has no attribute 'get_message_definition'

And this is not connected to this PR since it occurs also on master branch of pylint-django. I think it is connected to this commit:
pylint-dev/pylint@da67a9d#diff-9f2e05113cbb502c875114aa3287baef

I am not able to verify this PR until this is handled :-(

[carlio]

@matusvalo welcome to the world of trying to keep compatability between pylint upstream and users' configurations downstream :-) This is why I always push to have that compat.py even though it is gunge in the codebase it is unfortunately a necessary evil.

[matusvalo]

@carlio my opinion is that this is not ideal for users in any case: When user is not using latest version pylint-django he does not know which version of pylint he can use because setup.py specifies only lower bound:
https://github.com/PyCQA/pylint-django/blob/bb0153ba1388e5304d5c902efa0d260883e99d1a/setup.py#L21

E.g. for instance if pylint 2.3 is released and user uses combination of pylint 2.3 and pylint-django 2.0.5 he will get exception. Don't get me wrong. I am not saying that this should be fixed on pylint-django side - this would be very difficult to fix if not impossible. (I think better is to have stable API for plugins on pylint side).

[matusvalo]

@carlio @atodorov I am proposing the following implementation of warnings when "old" version of pylint is detected:

$ git diff
diff --git a/pylint_django/compat.py b/pylint_django/compat.py
index a461548..0393082 100644
--- a/pylint_django/compat.py
+++ b/pylint_django/compat.py
@@ -2,6 +2,10 @@
 # pylint: skip-file
 # no sane linter can figure out the hackiness in this compatability layer...

+import warnings
+
+warnings.simplefilter('always', DeprecationWarning)
+
 try:
     from astroid.nodes import ClassDef, FunctionDef, ImportFrom, AssignName, Attribute
 except ImportError:
@@ -26,3 +30,11 @@ import pylint

 # pylint before version 2.3 does not support load_configuration() hook.
 REGISTER_LEGACY = pylint.__pkginfo__.numversion < (2, 3)
+
+if REGISTER_LEGACY:
+    warnings.warn(
+        "Legacy pylint version is detected. Custom configuration is not supported. "
+        "Please upgrade pylint to version >=2.3. For more details see: "
+        "https://github.com/PyCQA/pylint-django/issues/181",
+        DeprecationWarning
+    )

For user it will be displayed as follows:

$ pylint --load-plugins pylint_django --good-names=foo foo/foo/
/home/d56032/environments/pylint36/lib/python3.6/site-packages/pylint_django-2.0.5-py3.6.egg/pylint_django/compat.py:39: DeprecationWarning: Legacy pylint version is detected. Custom configuration is not supported. Please upgrade pylint to version >=2.3. For more details see: https://github.com/PyCQA/pylint-django/issues/181
  DeprecationWarning
************* Module foo.urls
foo/foo/urls.py:19:0: C0103: Constant name "urlpatterns" doesn't conform to UPPER_CASE naming style (invalid-name)

------------------------------------------------------------------
Your code has been rated at 9.60/10 (previous run: 9.60/10, +0.00)

Are you fine with it? If yes I will add it to this PR.

[atodorov]

@matusvalo about your proposal for warnings:

It is out of scope for this PR IMO. I am open to discussing this in another PR or issue but please don't add it here. I don't mind the warning but honestly for people who need to use older versions for whatever reason this warning doesn't bring them any good.

@carlio - the above mentioned traceback may be a future breakage waiting to happen in pylint-plugin-utils. I'll try adding the same test config with pylint@master that we have here.

[matusvalo]

@atodorov I have implemented your comments.

[atodorov]

@matusvalo looks good, but you will have to squash all commits together and rebase to master.

[matusvalo]

@atodorov all commits are squashed now.

[atodorov]

Merged. I suggest we release a new version when pylint 2.3.0 comes out.