Fix too broad suppression of unused-argument warnings

CONTRIBUTORS.md

@@ -12,3 +12,4 @@
 * [matusvalo](https://github.com/matusvalo)
 * [fadedDexofan](https://github.com/fadeddexofan)
 * [imomaliev](https://github.com/imomaliev)
+* [psrb](https://github.com/psrb)

README.rst

@@ -124,6 +124,10 @@ a test name, and insert into it some code. The tests will run pylint
 against these modules. If the idea is that no messages now occur, then
 that is fine, just check to see if it works by running ``scripts/test.sh``.
 
+Any command line argument passed to ``scripts/test.sh`` will be passed to the internal invocation of ``pytest``.
+For example if you want to debug the tests you can execute ``scripts/test.sh --capture=no``.
+A specific test case can be run by filtering based on the file name of the test case ``./scripts/test.sh -k 'func_noerror_views'``.
+
 Ideally, add some pylint error suppression messages to the file to prevent
 spurious warnings, since these are all tiny little modules not designed to
 do anything so there's no need to be perfect.
@@ -132,7 +136,7 @@ It is possible to make tests with expected error output, for example, if
 adding a new message or simply accepting that pylint is supposed to warn.
 A ``test_file_name.txt`` file contains a list of expected error messages in the
 format
-``error-type:line number:class name or empty:1st line of detailed error text``.
+``error-type:line number:class name or empty:1st line of detailed error text:confidence or empty``.
 
 
 License

pylint_django/augmentations/__init__.py

@@ -644,23 +644,17 @@ def is_model_view_subclass_method_shouldnt_be_function(node):
     return parent is not None and node_is_subclass(parent, *subclass)
 
 
-def is_model_view_subclass_unused_argument(node):
+def ignore_unused_argument_warnings_for_request(orig_method, self, stmt, name):
     """
-    Checks that node is get or post method of the View class and it has valid arguments.
+    Ignore unused-argument warnings for function arguments named "request".
 
-    TODO: Bad checkings, need to be more smart.
+    The signature of Django view functions require the request argument but it is okay if the request is not used.
+    This function should be used as a wrapper for the `VariablesChecker._is_name_ignored` method.
     """
-    if not is_model_view_subclass_method_shouldnt_be_function(node):
-        return False
-
-    return is_argument_named_request(node)
+    if name == 'request':
+        return True
 
-
-def is_argument_named_request(node):
-    """
-    If an unused-argument is named 'request' ignore that!
-    """
-    return 'request' in node.argnames()
+    return orig_method(self, stmt, name)
 
 
 def is_model_field_display_method(node):
@@ -742,7 +736,7 @@ def is_class(class_name):
 def wrap(orig_method, with_method):
     @functools.wraps(orig_method)
     def wrap_func(*args, **kwargs):
-        with_method(orig_method, *args, **kwargs)
+        return with_method(orig_method, *args, **kwargs)
     return wrap_func
 
 
@@ -759,6 +753,31 @@ def pylint_newstyle_classdef_compat(linter, warning_name, augment):
     suppress_message(linter, getattr(NewStyleConflictChecker, 'visit_classdef'), warning_name, augment)
 
 
+def apply_wrapped_augmentations():
+    """
+    Apply augmentation and supression rules through monkey patching of pylint.
+    """
+    # NOTE: The monkey patching is done with wrap and needs to be done in a thread safe manner to support the
+    # parallel option of pylint (-j).
+    # This is achieved by comparing __name__ of the monkey patched object to the original value and only patch it if
+    # these are equal.
+
+    # Unused argument 'request' (get, post)
+    current_is_name_ignored = VariablesChecker._is_name_ignored  # pylint: disable=protected-access
+    if current_is_name_ignored.__name__ == '_is_name_ignored':
+        # pylint: disable=protected-access
+        VariablesChecker._is_name_ignored = wrap(current_is_name_ignored, ignore_unused_argument_warnings_for_request)
+
+    # ForeignKey and OneToOneField
+    current_leave_module = VariablesChecker.leave_module
+    if current_leave_module.__name__ == 'leave_module':
+        # current_leave_module is not wrapped
+        # Two threads may hit the next assignment concurrently, but the result is the same
+        VariablesChecker.leave_module = wrap(current_leave_module, ignore_import_warnings_for_related_fields)
+        # VariablesChecker.leave_module is now wrapped
+    # else VariablesChecker.leave_module is already wrapped
+
+
 # augment things
 def apply_augmentations(linter):
     """Apply augmentation and suppression rules."""
@@ -826,10 +845,6 @@ def apply_augmentations(linter):
     # Method could be a function (get, post)
     suppress_message(linter, ClassChecker.leave_functiondef, 'no-self-use',
                      is_model_view_subclass_method_shouldnt_be_function)
-    # Unused argument 'request' (get, post)
-    suppress_message(linter, VariablesChecker.leave_functiondef, 'unused-argument',
-                     is_model_view_subclass_unused_argument)
-    suppress_message(linter, VariablesChecker.leave_functiondef, 'unused-argument', is_argument_named_request)
 
     # django-mptt
     suppress_message(linter, DocStringChecker.visit_classdef, 'missing-docstring', is_model_mpttmeta_subclass)
@@ -841,15 +856,7 @@ def apply_augmentations(linter):
     suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_factory)
     suppress_message(linter, ClassChecker.visit_functiondef, 'no-self-argument', is_factory_post_generation_method)
 
-    # ForeignKey and OneToOneField
-    # Must update this in a thread safe way to support the parallel option on pylint (-j)
-    current_leave_module = VariablesChecker.leave_module
-    if current_leave_module.__name__ == 'leave_module':
-        # current_leave_module is not wrapped
-        # Two threads may hit the next assignment concurrently, but the result is the same
-        VariablesChecker.leave_module = wrap(current_leave_module, ignore_import_warnings_for_related_fields)
-        # VariablesChecker.leave_module is now wrapped
-    # else VariablesChecker.leave_module is already wrapped
-
     # wsgi.py
     suppress_message(linter, NameChecker.visit_assignname, 'invalid-name', is_wsgi_application)
+
+    apply_wrapped_augmentations()

pylint_django/tests/input/func_unused_arguments.py

@@ -0,0 +1,40 @@
+"""
+Checks that Pylint still complains about unused-arguments for other
+arguments if a function/method contains an argument named `request`.
+"""
+# pylint: disable=missing-docstring
+
+from django.http import JsonResponse
+from django.views import View
+
+# Pylint generates the warning `redefined-outer-name` if an argument name shadows
+# a variable name from an outer scope. But if that argument name is ignored this
+# warning will not be generated.
+# Therefore define request here to cover this behaviour in this test case.
+
+request = None  # pylint: disable=invalid-name
+
+
+def user_detail(request, user_id):  # [unused-argument]
+    # nothing is done with user_id
+    return JsonResponse({'username': 'steve'})
+
+
+class UserView(View):
+    def get(self, request, user_id):  # [unused-argument]
+        # nothing is done with user_id
+        return JsonResponse({'username': 'steve'})
+
+
+# The following views are already covered in other test cases.
+# They are included here for completeness sake.
+
+def welcome_view(request):
+    # just don't use `request' b/c we could have Django views
+    # which never use it!
+    return JsonResponse({'message': 'welcome'})
+
+
+class CBV(View):
+    def get(self, request):
+        return JsonResponse({'message': 'hello world'})

pylint_django/tests/input/func_unused_arguments.txt

@@ -0,0 +1,2 @@
+unused-argument:18:user_detail:Unused argument 'user_id':HIGH
+unused-argument:24:UserView.get:Unused argument 'user_id':INFERENCE

pylint_django/tests/test_func.py

@@ -29,6 +29,7 @@ class PylintDjangoLintModuleTest(test_functional.LintModuleTest):
     def __init__(self, test_file):
         super(PylintDjangoLintModuleTest, self).__init__(test_file)
         self._linter.load_plugin_modules(['pylint_django'])
+        self._linter.load_plugin_configuration()
 
 
 class PylintDjangoDbPerformanceTest(PylintDjangoLintModuleTest):
@@ -39,6 +40,7 @@ class PylintDjangoDbPerformanceTest(PylintDjangoLintModuleTest):
     def __init__(self, test_file):
         super(PylintDjangoDbPerformanceTest, self).__init__(test_file)
         self._linter.load_plugin_modules(['pylint_django.checkers.db_performance'])
+        self._linter.load_plugin_configuration()
 
 
 def get_tests(input_dir='input', sort=False):

scripts/test.sh

@@ -1,2 +1,2 @@
 #!/bin/bash
-python pylint_django/tests/test_func.py -v
+python pylint_django/tests/test_func.py -v "$@"