Fix typo in pylintrc for useless-suppression #5303

DanielNoord · 2021-11-14T12:07:44Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #5040

See discussion there.

coveralls · 2021-11-14T12:13:40Z

Pull Request Test Coverage Report for Build 1494784110

3 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage remained the same at 93.453%

Files with Coverage Reduction	New Missed Lines	%
pylint/extensions/overlapping_exceptions.py	1	84.09%

Totals
Change from base Build 1493950870:	0.0%
Covered Lines:	13932
Relevant Lines:	14908

💛 - Coveralls

Pierre-Sassoulas

👍

DanielNoord · 2021-11-14T12:28:29Z

@Pierre-Sassoulas Isn't this supposed to show some useless suppressions in our current code? I remember I saw some errors previously. Did we fix those in another pr?

Pierre-Sassoulas · 2021-11-14T14:49:33Z

Did we fix those in another pr?

Hmm, I don't think so, there must be something wrong with the way we launch pylint in github actions because:

pylint pylint/ --enable=all|grep useless

pylint/utils/utils.py:96:0: I0021: Useless suppression of 'redefined-builtin' (useless-suppression)
pylint/utils/file_state.py:77:0: I0021: Useless suppression of 'not-callable' (useless-suppression)
pylint/utils/file_state.py:79:0: I0021: Useless suppression of 'no-member' (useless-suppression)
pylint/lint/pylinter.py:621:0: I0021: Useless suppression of 'consider-using-with' (useless-suppression)
pylint/lint/pylinter.py:1064:0: I0021: Useless suppression of 'no-member' (useless-suppression)
pylint/lint/run.py:24:0: I0021: Useless suppression of 'not-callable' (useless-suppression)
pylint/config/option.py:184:0: I0021: Useless suppression of 'unsupported-assignment-operation' (useless-suppression)
pylint/reporters/text.py:296:0: I0021: Useless suppression of 'import-error' (useless-suppression)
pylint/checkers/utils.py:1346:0: I0021: Useless suppression of 'redundant-keyword-arg' (useless-suppression)
pylint/checkers/similar.py:30:0: I0021: Useless suppression of 'redefined-builtin' (useless-suppression)
pylint/checkers/format.py:58:0: I0021: Useless suppression of 'redefined-builtin' (useless-suppression)
pylint/checkers/variables.py:1788:0: I0021: Useless suppression of 'too-many-branches' (useless-suppression)
pylint/testutils/reporter_for_tests.py:53:0: I0021: Useless suppression of 'unused-argument' (useless-suppression)
pylint/testutils/unittest_linter.py:17:0: I0021: Useless suppression of 'no-self-use' (useless-suppression)
pylint/testutils/lint_module_test.py:149:0: I0021: Useless suppression of 'consider-using-with' (useless-suppression)
pylint/testutils/lint_module_test.py:156:0: I0021: Useless suppression of 'consider-using-with' (useless-suppression)

cdce8p · 2021-11-14T18:26:55Z

Hmm, I don't think so, there must be something wrong with the way we launch pylint in github actions

Try adding the -v flag to the pre-commit pylint run. Normally the log is only shown if the script exits with != 0.
https://pre-commit.com/#pre-commit-run

Pierre-Sassoulas · 2021-11-14T20:52:04Z

Ho is this the issue where I type message exit with zero even when there are some ? We could change the pylint option when we launch, but this feel wrong, if some message are emitted we should exit with an error code. I can't find the issue about it but I read about that problem somewhere.

cdce8p · 2021-11-14T21:26:27Z

Ho is this the issue where I type message exit with zero even when there are some ? We could change the pylint option when we launch, but this feel wrong, if some message are emitted we should exit with an error code. I can't find the issue about it but I read about that problem somewhere.

We discussed it here: pylint-dev/astroid#1217 (comment). As I pointed out then we should not emit a non-zero exit code for useless-suppression messages. At least not without some more investigations.

The issue is that some errors might be Python version specific. It could create a situation where only one, pre-commit or CI, will succeed when tested with different versions. The py-version option should help, but I don't know if it's enough.

Additionally, this would prevent us from having the latest pylint version installed in a local env as this is likely to have fixed some false-positives. -> Which would then trigger useless-suppression.

DanielNoord · 2021-11-15T14:39:07Z

I added a fix with allows failing on Information messages.

It requires changing how fail_under works. Currently we can get a lower score than 10 by adding Info messages to the evaluation, but since that exit code is 0 we won't actually produce a non-zero exit.
A couple of lines above we fixed it likes this, so I imagine this might work here as well?

@cdce8p Our CI takes into account our pylintrc which has a py-version (3.6). So that shouldn't create problems (I think?). I think the problem of local installations of pylint also doesn't affect us here, since pre-commit for pylint should always run with the locally installed pylint anyway?

This requires careful thought, but I think we might be fine by adding this. If agreed I will fix the outstanding useless-suppression warnings as well so this can go in 2.13.

Pierre-Sassoulas · 2021-11-15T15:09:58Z

only one, pre-commit or CI, will succeed when tested with different versions

Isn't this is a good thing and a way to find problem with py-version ? If we actually check that there is no useless-suppression for any interpreter, then we can cheaply detect inconsistencies ?

cdce8p · 2021-11-15T21:02:48Z

@cdce8p Our CI takes into account our pylintrc which has a py-version (3.6). So that shouldn't create problems (I think?). I think the problem of local installations of pylint also doesn't affect us here, since pre-commit for pylint should always run with the locally installed pylint anyway?

Isn't this is a good thing and a way to find problem with py-version ? If we actually check that there is no useless-suppression for any interpreter, then we can cheaply detect inconsistencies ?

I guess we'll find out if it works. It was one of the reasons to add such a setting in the first place. Might be that my reservations are unfound.

cdce8p · 2021-11-15T21:05:27Z

.pre-commit-config.yaml

+            "--fail-under=10",
+            "--evaluation='10.0 - ((float(5 * error + warning + refactor + convention +
+            info) / statement) * 10)'",


Why not use --fail-on=useless-suppression instead?
Maybe --fail-on=I works 🤷🏻‍♂️ haven't tested it.

--fail-on works, forgot that was a thing!

pylint/lint/run.py

DanielNoord · 2021-11-15T22:47:40Z

pylint/utils/file_state.py

@@ -74,9 +74,9 @@ def _collect_block_lines(
        #
        # 1.   def meth8(self):
        # 2.        """test late disabling"""
-        # 3.        pylint: disable=not-callable
+        # 3.        pylint: disable=not-callable, useless-suppression


I think this is the most elegant way to allow us to still fail on I.

DanielNoord · 2021-11-15T22:58:42Z

This does indeed create issues 😢

https://github.com/PyCQA/pylint/blob/8a653aab1580d65f30d3575c7a482665598f5016/pylint/lint/run.py#L24-L29

On 3.10 using-constant-test is useless but on 3.6 (which the pre-commit CI uses) it is not. This begs the question whether we should add useless-suppression to our pylintrc at all. In this case it will always create a warning on either 3.6 or 3.10. Although I expect most contributors to be on >3.8 we can't really say "pylint supports 3.6", but "pylint development should be done on 3.8+"...

Pierre-Sassoulas · 2021-11-16T07:29:09Z

"pylint development should be done on 3.8+".

It's kinda true though, as the AST changed in python 3.8, if you need to work on proper column index for messages you need 3.8+ (functional tests will help you but you will need to check the expected column by running github actions and you won't be able to update the column automatically). So maybe we could upgrade the interpreter we use for pre-commit to 3.8 and upgrade the contributor documentation accordingly.

cdce8p · 2021-11-16T22:49:38Z

So maybe we could upgrade the interpreter we use for pre-commit to 3.8 and upgrade the contributor documentation accordingly.

Up until just recently my goto recommendation would have been to use the oldest supported Python version at least for mypy and pylint. It might work with 3.8, especially considering our extensive test suite. However there is always a small risk that something goes undetected. I'm thinking especially about no-member errors. I.e. we try to add functions that aren't available in all supported versions -> cached_property 😉

DanielNoord · 2021-11-23T08:26:13Z

I'm thinking especially about no-member errors. I.e. we try to add functions that aren't available in all supported versions -> cached_property 😉

Shouldn't those still fail in the regular test suite?

To make sure this PR doesn't stay open indefinitely I propose to:

Update our pre-commit CI to run on 3.8
Add a changelog entry and doc entry that it is recommended to work on pylint on 3.8+

I think because of the aforementioned change to the ast parser that makes sense. The entry and this change can then go in 2.12 as it is the easiest way to communicate this to potential developers. (If we put it in 2.13 we should officially wait till its release before we can change the CI environment).

Pierre-Sassoulas · 2021-11-23T08:44:11Z

Shouldn't those still fail in the regular test suite?

Yes, we still run all tests on 3.6 / 3.7 we'd catch the real error. Whatever we do automated tests are still better than static analysis 😄

Update our pre-commit CI to run on 3.8
Add a changelog entry and doc entry that it is recommended to work on pylint on 3.8+

👍

DanielNoord · 2021-11-23T11:06:17Z

pylint/lint/run.py

@@ -21,7 +21,7 @@
 def _cpu_count() -> int:
    """Use sched_affinity if available for virtualized or containerized environments."""
    sched_getaffinity = getattr(os, "sched_getaffinity", None)
-    # pylint: disable=not-callable,using-constant-test
+    # pylint: disable=not-callable,using-constant-test,useless-suppression


This is unsolvable atm. The pre-commit env does have sched_getaffinity so this will always create some sort of error.

Pierre-Sassoulas

LGTM !

doc/whatsnew/2.12.rst

Pierre-Sassoulas · 2021-11-23T12:41:03Z

pylint/utils/utils.py

@@ -93,7 +93,7 @@ def normalize_text(text, line_len=DEFAULT_LINE_LENGTH, indent=""):


 # py3k has no more cmp builtin
-def cmp(a, b):  # pylint: disable=redefined-builtin


This was useless since pylint 2.0 😄

ChangeLog

Co-authored-by: Pierre Sassoulas <[email protected]>

Fix typo in pylintrc for useless-suppression

5f9ec8d

DanielNoord added Maintenance Discussion or action around maintaining pylint or the dev workflow Minor 💅 Polishing pylint is always nice labels Nov 14, 2021

Pierre-Sassoulas approved these changes Nov 14, 2021

View reviewed changes

Pierre-Sassoulas added this to the 2.13.0 milestone Nov 14, 2021

Fix CI for information messages

ce47794

cdce8p reviewed Nov 15, 2021

View reviewed changes

Remove useless disables

4c9aebc

DanielNoord commented Nov 15, 2021

View reviewed changes

DanielNoord modified the milestones: 2.13.0, 2.12.2 Nov 15, 2021

DanielNoord mentioned this pull request Nov 15, 2021

Allow failing when score is affected by Information warning #5318

Merged

2 tasks

DanielNoord added 2 commits November 23, 2021 09:54

Merge branch 'main' of https://github.com/PyCQA/pylint into suppression

e3442aa

Update CI and docs for 3.8+

2f77061

DanielNoord added the task label Nov 23, 2021

Revert change

249c7a1

DanielNoord modified the milestones: 2.12.2, 2.12.0 Nov 23, 2021

DanielNoord added 2 commits November 23, 2021 10:24

Update ignore

37ed013

Fix

8fd3c5f

DanielNoord commented Nov 23, 2021

View reviewed changes

DanielNoord marked this pull request as ready for review November 23, 2021 11:06

DanielNoord requested a review from Pierre-Sassoulas November 23, 2021 11:07

Pierre-Sassoulas approved these changes Nov 23, 2021

View reviewed changes

Apply suggestions from code review

66ea9a5

Co-authored-by: Pierre Sassoulas <[email protected]>

Pierre-Sassoulas merged commit d58a7c0 into pylint-dev:main Nov 23, 2021

DanielNoord deleted the suppression branch November 24, 2021 07:25

Pierre-Sassoulas removed the task label Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typo in pylintrc for useless-suppression #5303

Fix typo in pylintrc for useless-suppression #5303

DanielNoord commented Nov 14, 2021

coveralls commented Nov 14, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

DanielNoord commented Nov 14, 2021

Pierre-Sassoulas commented Nov 14, 2021

cdce8p commented Nov 14, 2021

Pierre-Sassoulas commented Nov 14, 2021

cdce8p commented Nov 14, 2021

DanielNoord commented Nov 15, 2021

Pierre-Sassoulas commented Nov 15, 2021

cdce8p commented Nov 15, 2021

cdce8p Nov 15, 2021

DanielNoord Nov 15, 2021

DanielNoord Nov 15, 2021

DanielNoord commented Nov 15, 2021

Pierre-Sassoulas commented Nov 16, 2021

cdce8p commented Nov 16, 2021

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas commented Nov 23, 2021

DanielNoord Nov 23, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas Nov 23, 2021

		@@ -93,7 +93,7 @@ def normalize_text(text, line_len=DEFAULT_LINE_LENGTH, indent=""):


		# py3k has no more cmp builtin
		def cmp(a, b): # pylint: disable=redefined-builtin

Fix typo in pylintrc for useless-suppression #5303

Fix typo in pylintrc for useless-suppression #5303

Conversation

DanielNoord commented Nov 14, 2021

Type of Changes

Description

coveralls commented Nov 14, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1494784110

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Nov 14, 2021

Pierre-Sassoulas commented Nov 14, 2021

cdce8p commented Nov 14, 2021

Pierre-Sassoulas commented Nov 14, 2021

cdce8p commented Nov 14, 2021

DanielNoord commented Nov 15, 2021

Pierre-Sassoulas commented Nov 15, 2021

cdce8p commented Nov 15, 2021

cdce8p Nov 15, 2021

Choose a reason for hiding this comment

DanielNoord Nov 15, 2021

Choose a reason for hiding this comment

DanielNoord Nov 15, 2021

Choose a reason for hiding this comment

DanielNoord commented Nov 15, 2021

Pierre-Sassoulas commented Nov 16, 2021

cdce8p commented Nov 16, 2021

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas commented Nov 23, 2021

DanielNoord Nov 23, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Nov 23, 2021

Choose a reason for hiding this comment

coveralls commented Nov 14, 2021 •

edited

Loading