Fix too broad suppression of unused-argument warnings
#251
Conversation
psrb
changed the title
unused-argument warnings
Pull Request Test Coverage Report for Build 860
💛 - Coveralls |
|
Can you rebase on top of #252 so we can see that tests work while we wait for it to get reviewed/merged ? |
psrb
force-pushed
the
bugfix-request-unused-arguments
branch
2 times, most recently
from
1cf723f
to
c817213
Compare
README.rst
Outdated
| @@ -124,6 +124,9 @@ 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``. | |||
|
|
|||
| You can pass any pytest command line argument to ``scripts/test.sh``. | |||
There was a problem hiding this comment.
Hmm, the wording may be confusing or I understand something wrong, but I still think that it should be pytest, not python:
$ ./scripts/test.sh --version
This is pytest version 5.2.2, imported from ...My suggestion to hopefully make it a clearer:
Any command line argument passed to
scripts/test.shwill be passed to the internal invocation ofpytest. For example if you want to debug the tests you can executescripts/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_unused_arguments'.
There was a problem hiding this comment.
Ohh, I must be blind. I was looking at what test.sh executes and totally missed the part that we call pytest.main() in test_func.py.
+1 for the last suggestion, it does make things very clear & sorry for the noise.
psrb
force-pushed
the
bugfix-request-unused-arguments
branch
from
b273a43
to
d564872
Compare
Previously you had to modify scripts/test.sh to add command line arguments to pytest. Now you can add them directly when calling test.sh. This is useful if you, for example, want to filter the executed tests using `-k`.
When running the tests the plugin was loaded but its `load_configuration` function was not called. The function needs to be called so changes to config entries can be tested.
…v#249) The previous implementation (result of pylint-dev#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.
psrb
force-pushed
the
bugfix-request-unused-arguments
branch
from
d564872
to
a7cb1e1
Compare
|
I am done now. I have rebased the branch onto the current master and tidied the commit history. After a final review this can be merged. |
This PR introduces the following changes:
Pass arguments provided to scripts/test.sh along to test_func/pytest
Previously you had to modify
scripts/test.shto pass new arguments topytest. Now you can callscripts/test.sh --capture=noif you want to drop into the debugger when running tests 😃Ensure that load_configuration of pylint-django is called when running tests
Changes made in
load_configurationcan now be tested.The main part of this PR is the bugfix for #249
As described in #249 the suppression of the
unused-argumentwarning if a function contains an argument namedrequestwas too broad.Implementation
ignored-argument-namesregex to includedrequest. This way otherunused-argumentwarnings will still be issued!INTERFERENCEand not the default value ofHIGH.Note on failing tests
I have tested my changes locally using Pylint 2.3.1. The CI pipeline currently fails because of changes made in the Pylint repository (see #250 for more information).