From a7cb1e14f948cd6828709458afef4ff1a7ff4da0 Mon Sep 17 00:00:00 2001 From: Pascal Urban Date: Sun, 27 Oct 2019 23:00:57 +0100 Subject: [PATCH] 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. --- CONTRIBUTORS.md | 1 + README.rst | 2 +- pylint_django/augmentations/__init__.py | 63 ++++++++++--------- .../tests/input/func_unused_arguments.py | 40 ++++++++++++ .../tests/input/func_unused_arguments.txt | 2 + 5 files changed, 79 insertions(+), 29 deletions(-) create mode 100644 pylint_django/tests/input/func_unused_arguments.py create mode 100644 pylint_django/tests/input/func_unused_arguments.txt diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index f44a3922..6f1fc917 100644 --- a/CONTRIBUTORS.md +++ b/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) diff --git a/README.rst b/README.rst index 20d07c32..874671d8 100644 --- a/README.rst +++ b/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 diff --git a/pylint_django/augmentations/__init__.py b/pylint_django/augmentations/__init__.py index 17757d40..3f7ac9bf 100644 --- a/pylint_django/augmentations/__init__.py +++ b/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() diff --git a/pylint_django/tests/input/func_unused_arguments.py b/pylint_django/tests/input/func_unused_arguments.py new file mode 100644 index 00000000..61b2e23a --- /dev/null +++ b/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'}) diff --git a/pylint_django/tests/input/func_unused_arguments.txt b/pylint_django/tests/input/func_unused_arguments.txt new file mode 100644 index 00000000..cd3d120e --- /dev/null +++ b/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