Ignore unused-argument warning for request arguments (Fixes #249)

The previous implementation (result of #155) of disabling the
unused-argument warning for functions/methods having an argument named
`request` was to broad. As soon as a function/method contains a
`request` argument no unused-argument warning will be issued for this
function anymore, even though some other arguments are unused!

This commit monkey patches `VariablesChecker._is_ignored_named` to
specifically suppress the generation of unused-argument warnings for
arguments named `request`.
This way unused-argument warnings will still be issued for every other
unused argument of the function/method.

Author: psrb

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

@@ -136,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