Rename `check_messages` #6196

DudeNr33 · 2022-04-05T17:52:58Z

Write a good description on what the PR does.
If you used multiple emails or multiple names when contributing, add your mails
and preferred name in script/.contributors_aliases.json

Type of Changes

	Type
	🐛 Bug fix
	✨ New feature
✓	🔨 Refactoring
✓	📜 Docs

Description

First step for cleaning up @check_messages.
Rename the decorator and improve the docstring.
Add alias for backwards compatibility, but do not emit a DeprecationWarning yet before we have not removed/renamed all usages ourselves.

I chose to not include it in the official documentation, as @check_messages was not mentioned as well. I think that most custom checkers would be very focused and would not benefit much from the decorator anyway.

Closes #6060

…kwards compatibility.

DanielNoord

Since this is mostly for internal use and we don't really need to worry about how pretty the function name is, what about checker_only_required_for? Or is that overkill?

pylint/checkers/utils.py

coveralls · 2022-04-05T18:01:13Z

Pull Request Test Coverage Report for Build 2176347274

9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
68 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.005%) to 95.043%

Files with Coverage Reduction	New Missed Lines	%
pylint/testutils/functional/test_file.py	2	96.49%
pylint/checkers/refactoring/refactoring_checker.py	11	98.26%
pylint/checkers/classes/class_checker.py	18	94.25%
pylint/checkers/variables.py	18	96.64%
pylint/testutils/lint_module_test.py	19	86.44%

Totals
Change from base Build 2174016644:	0.005%
Covered Lines:	15685
Relevant Lines:	16503

💛 - Coveralls

DudeNr33 · 2022-04-05T18:02:38Z

checker_only_required_for sounds to me as if this decorator is meant to be applied to the checker class. Maybe only_checks_messages or only_required_for_messages?

Co-authored-by: Daniël van Noord <[email protected]>

DanielNoord · 2022-04-05T18:03:23Z

checker_only_required_for sounds to me as if this decorator is meant to be applied to the checker class. Maybe only_checks_messages or only_required_for_messages?

+1 for only_required_for_messages

Pierre-Sassoulas

Looks great. Should we also migrate everything in this MR ?

Pierre-Sassoulas · 2022-04-05T18:06:17Z

pylint/checkers/utils.py

+
+    Use only_required_for instead, which conveys the intent of the decorator much clearer.
+    """
+    # Uncomment the following warning once all 'check_messages' calls have been replaced


I think we can activate it right now so we see if we migrated everything by seeing the warnings in tests :)

If this does not lead to a failed CI pipeline, we can activate it right away. 👍

I think for some deprecation warnings it's hard to deactivate them (or would take too much time to catch them everywhere), so warning do not make the CI fail.

DudeNr33 · 2022-04-05T19:01:45Z

If we want to just rename all occurrences of check_messages in the first step we can do that here.
If we want to combine it with actually checking if the decorator is applied correctly, I would do it checker per checker, as it would be impossible to properly review it in a single PR.

DudeNr33 · 2022-04-09T15:57:07Z

pylint/checkers/utils.py

+    *messages: str,
+) -> Callable[
+    [Callable[[Any, nodes.NodeNG], None]], Callable[[Any, nodes.NodeNG], None]
+]:


@DanielNoord the typing gets really longish here. Are we sure that we want to keep it?

Also, I initially tried to put BaseChecker as first argument instead of Any, but even though I think this should be correct mypy failed, because we don't have BaseChecker instances but instances of subclasses.

Maybe we need to do something like this:

from typing import TypeVar C = TypeVar('C', bound=BaseChecker)

[Callable[[C, nodes.NodeNG], None]], Callable[[C, nodes.NodeNG], None]

Based on https://mypy.readthedocs.io/en/stable/generics.html (The Friend / SuperFriend example)

Thanks, that worked!

DudeNr33 · 2022-04-09T15:57:54Z

@Pierre-Sassoulas let me know if you want me to do the renaming directly in this PR. :)

Pierre-Sassoulas

LGTM, let's separate the deprecation and the migration as you said it's clearer.

pylint/checkers/utils.py

DanielNoord · 2022-04-11T08:54:41Z

tests/utils/unittest_ast_walker.py

 import astroid

-from pylint.checkers.utils import check_messages
+from pylint.checkers.utils import only_required_for_messages


Shall we add one test with pytest.warns(DeprecationWarning)? That means that coverage will remain 100% for the check_messages function and tests the deprecation.

Yes, that makes sense - will do!

Co-authored-by: Daniël van Noord <[email protected]>

DudeNr33 · 2022-04-15T12:05:47Z

@DanielNoord any idea why the tests are failing on Python 3.8 and below?

ImportError while loading conftest '/home/runner/work/pylint/pylint/tests/conftest.py'.
tests/conftest.py:[11](https://github.com/PyCQA/pylint/runs/6037209987?check_suite_focus=true#step:7:11): in <module>
    from pylint import checkers
pylint/checkers/__init__.py:49: in <module>
    from pylint.checkers.deprecated import DeprecatedMixin
pylint/checkers/deprecated.py:[16](https://github.com/PyCQA/pylint/runs/6037209987?check_suite_focus=true#step:7:16): in <module>
    from pylint.checkers import utils
pylint/checkers/utils.py:228: in <module>
    AstCallback = Callable[[CheckerT, T_Node], None]
E   TypeError: 'ABCMeta' object is not subscriptable

DanielNoord · 2022-04-15T12:33:52Z

pylint/checkers/utils.py


 T_Node = TypeVar("T_Node", bound=nodes.NodeNG)
+CheckerT = TypeVar("CheckerT", bound=BaseChecker)
+AstCallback = Callable[[CheckerT, T_Node], None]


You'll need to import Callable from typing I think. Whenever it is used in runtime it needs to be from typing (until 3.9).

It is already imported from typing.
I could solve it locally with putting it inside a if TYPE_CHECKING block. Let's see if the CI is happy with that.

…on 3.8 and below

for more information, see https://pre-commit.ci

DanielNoord · 2022-04-16T09:59:55Z

I checked locally and importing from typing did work on 3.7. Sorry for pushing to your branch but I'd like to see if we can do this without the import guard.

See pylint-dev/pylint#6196 Change-Id: I28ef1b0ea9911451699aeee7f78be4949bde1224

Rename check_messages, improve docstring and create alias for bac…

7eb14b2

…kwards compatibility.

DanielNoord requested changes Apr 5, 2022

View reviewed changes

pylint/checkers/utils.py Outdated Show resolved Hide resolved

pylint/checkers/utils.py Outdated Show resolved Hide resolved

Apply suggestions from code review

7645846

Co-authored-by: Daniël van Noord <[email protected]>

Pierre-Sassoulas reviewed Apr 5, 2022

View reviewed changes

Pierre-Sassoulas added Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Apr 5, 2022

Pierre-Sassoulas added this to the 2.14.0 milestone Apr 5, 2022

DudeNr33 added 3 commits April 9, 2022 17:27

Rename function to clarify intent. Activate DeprecationWarning

58f57ce

Fix tests

49cfc9f

Fix type hint

f333f40

DudeNr33 commented Apr 9, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Apr 9, 2022

View reviewed changes

Add TypeVars for BaseChecker subclasses and AST callbacks

135151a

DudeNr33 requested a review from DanielNoord April 10, 2022 10:56

DanielNoord reviewed Apr 11, 2022

View reviewed changes

DudeNr33 and others added 2 commits April 15, 2022 12:28

Apply suggestions from code review

bc4d7f9

Co-authored-by: Daniël van Noord <[email protected]>

Add unittest for deprecation warning

21200b8

DanielNoord reviewed Apr 15, 2022

View reviewed changes

DudeNr33 and others added 4 commits April 15, 2022 15:23

Move type hints into guard clause to avoid runtime problems with Pyth…

68d25e1

…on 3.8 and below

Merge branch 'main' into rename-check-messages

c241a70

[pre-commit.ci] auto fixes from pre-commit.com hooks

3b89ba5

for more information, see https://pre-commit.ci

Fix reimports introduced by merge commit

4751d38

DudeNr33 requested a review from DanielNoord April 16, 2022 09:09

Change typing

9fd8d7c

DanielNoord approved these changes Apr 16, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Apr 16, 2022

View reviewed changes

Pierre-Sassoulas merged commit 0bd2e5a into pylint-dev:main Apr 16, 2022

Pierre-Sassoulas mentioned this pull request Apr 20, 2022

Migrate from 'check_messages' to 'only_required_for_messages' #6417

Merged

Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022

CheckmkCI pushed a commit to Checkmk/checkmk that referenced this pull request Oct 11, 2023

Track renaming of a pylint utility function.

327616a

See pylint-dev/pylint#6196 Change-Id: I28ef1b0ea9911451699aeee7f78be4949bde1224

Uh oh!

Rename check_messages #6196

Rename check_messages #6196

Uh oh!

Conversation

DudeNr33 commented Apr 5, 2022

Type of Changes

Description

Uh oh!

DanielNoord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2176347274

💛 - Coveralls

Uh oh!

DudeNr33 commented Apr 5, 2022

Uh oh!

DanielNoord commented Apr 5, 2022

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DudeNr33 commented Apr 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DudeNr33 commented Apr 9, 2022

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DudeNr33 commented Apr 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Apr 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rename `check_messages` #6196

Rename `check_messages` #6196

coveralls commented Apr 5, 2022 •

edited

Loading