Move message adding functions from `MessagesHandlerMixIn` #5129

DanielNoord · 2021-10-07T19:26:32Z

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

Type of Changes

	Type
✓	🔨 Refactoring

Description

Ref #5018
Almost there 😄

This should be second-to-last PR and includes all functions related to adding message to the reporter.
I'll review and annotate the code because there are some hard to spot changes included with this move.

Sorry for the rebase spam. Made two silly (spelling) mistakes in comments 😅

DanielNoord · 2021-10-07T19:29:59Z

pylint/lint/pylinter.py

@@ -519,7 +536,11 @@ def __init__(self, options=(), reporter=None, option_groups=(), pylintrc=None):
            "disable-msg": self._options_methods["disable"],
            "enable-msg": self._options_methods["enable"],
        }
-        MessagesHandlerMixIn.__init__(self)
+
+        # Attributes related to message (state) handling


These come from MessagesHandlerMixIn.init(). They could be their own PR but I thought this fitted.

DanielNoord · 2021-10-07T19:30:48Z

pylint/lint/pylinter.py

@@ -1300,3 +1321,196 @@ def _report_evaluation(self):
            sect = report_nodes.EvaluationSection(msg)
            self.reporter.display_reports(sect)
        return note
+
+    # Adding (ignored) messages to the Message Reporter


I couldn't find a better way to indicate a section of functions without running into https://www.flake8rules.com/rules/E266.html

I really dislike how # IAstroidChecker interface ################################################# looks on L1248.

We need separators because we have a badly designed god class with 1500+ lines, I would classify having a nicer looking separator as "polishing a turd". Granted we're probably not re-designing pylinter any time soon but... well I think this separator is good enough.

I agree, although I don't immediately know how to redesign PyLinter without passing self as a parameter to functions that are now methods. A lot of its methods require access to self...

This separator as in the one I added right now? Or do you want me to add the additional #?

The separator are good enough let's leave them all as is :)

DanielNoord · 2021-10-07T19:31:46Z

pylint/lint/pylinter.py

+
+    # Adding (ignored) messages to the Message Reporter
+
+    def _get_message_state_scope(


This is now a private method. The # type: ignore's are necessary because mypy doesn't infer that the return values are constants and complains that the return value should be Optional[int].

It seems we're testing this function in the tests. Generally, I would say that if a function is private you don't need to test it directly and can test it through the public function. Could it be a subclass with message related code instead (MessageHandler ?) ? Then we'd be using that class in Pylinter using composition instead of inheritance and we'd be able to test it without accessing private function (and pylinter would be more manageable).

I tried to do that at some point and there was a problem because some function are called with self being a MessageHandlerMixin at some point and then a Pylinter at another point. Which as you said is bad design, (but also hard to refactor). What do you think ?

I looked at this but we need self.file_state._module_msgs_state[msgid]: at L1339 here. As well as self.config on L1336. We could pass self as parameter, but I think that would be part of a bigger redesign (see previous comment). For now I think removing MessagesHandlerMixIn is a good first step.

I don't necessarily agree with the private method testing, but I also don't know what is standard for other python projects. I feel like it is always good to test a function directly as you can diagnose potential problems directly. If we test this by testing the functions that call it and those tests fail we will need an additional step to diagnose the problem. Testing private methods directly makes it easier to diagnose regressions.

It's not a Python standard, it's a general standard. It means you can modify private function without changing tests or code. Ie implementation details are hidden (for example move from a polar representation to cartesian in a coordinate class without changing the interface). Or if it should be public and his hidden to simplify the class api it forces to put the code in another code unit that is tested itself, smaller and simpler. (I suppose you made the function private to simplify pylinter's api but i'd like to know your rationale if that's not it)

Anyway, seeing the complexity of pylinter, it's not the time to aim for theorical purity.

Understood!

Is this good to go then?

DanielNoord · 2021-10-07T19:32:04Z

pylint/lint/pylinter.py

+            return MSG_STATE_SCOPE_CONFIG  # type: ignore
+        return None
+
+    def _is_one_message_enabled(self, msgid: str, line: Optional[int]) -> bool:


This is now a private method

DanielNoord · 2021-10-07T19:32:34Z

pylint/lint/pylinter.py

+                return True
+        return False
+
+    def _add_one_message(


This is now a private method. Note that args remains Any, to be tackled at a different time.

DanielNoord · 2021-10-07T19:33:27Z

pylint/lint/pylinter.py

+                        if message_line <= line
+                    ]
+                )
+                _, fallback_iter = next(closest_lines, (None, None))


I changed how this works because mypy was unable to follow what was happening here. The results remains the same.

DanielNoord · 2021-10-07T19:33:54Z

tests/lint/unittest_lint.py

@@ -316,14 +316,14 @@ class FakeConfig:

    linter = init_linter
    linter.disable("C0202")
-    assert MSG_STATE_SCOPE_CONFIG == linter.get_message_state_scope("C0202")
+    assert MSG_STATE_SCOPE_CONFIG == linter._get_message_state_scope("C0202")


To accommodate the change to the method being private.

coveralls · 2021-10-07T19:34:36Z

Pull Request Test Coverage Report for Build 1317624768

90 of 91 (98.9%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage remained the same at 93.199%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/lint/pylinter.py	87	88	98.86%

Files with Coverage Reduction	New Missed Lines	%
pylint/message/message_handler_mix_in.py	1	93.59%

Totals
Change from base Build 1317385477:	0.0%
Covered Lines:	13539
Relevant Lines:	14527

💛 - Coveralls

Pierre-Sassoulas

👍

DanielNoord force-pushed the move-add-message branch from 753314a to 1f080c0 Compare October 7, 2021 19:27

Move message adding functions from MessagesHandlerMixIn

38df06e

DanielNoord force-pushed the move-add-message branch from 1f080c0 to 38df06e Compare October 7, 2021 19:28

DanielNoord commented Oct 7, 2021

View reviewed changes

DanielNoord requested a review from Pierre-Sassoulas October 9, 2021 16:19

Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Oct 10, 2021

Pierre-Sassoulas approved these changes Oct 10, 2021

View reviewed changes

DanielNoord merged commit 1eaf5aa into pylint-dev:main Oct 10, 2021

DanielNoord deleted the move-add-message branch October 10, 2021 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move message adding functions from `MessagesHandlerMixIn` #5129

Move message adding functions from `MessagesHandlerMixIn` #5129

DanielNoord commented Oct 7, 2021 •

edited

Loading

DanielNoord Oct 7, 2021

DanielNoord Oct 7, 2021

Pierre-Sassoulas Oct 9, 2021

DanielNoord Oct 9, 2021

Pierre-Sassoulas Oct 10, 2021

DanielNoord Oct 7, 2021

Pierre-Sassoulas Oct 9, 2021 •

edited

Loading

DanielNoord Oct 9, 2021

Pierre-Sassoulas Oct 10, 2021 •

edited

Loading

DanielNoord Oct 10, 2021

DanielNoord Oct 7, 2021

DanielNoord Oct 7, 2021

DanielNoord Oct 7, 2021

DanielNoord Oct 7, 2021

coveralls commented Oct 7, 2021 •

edited

Loading

Pierre-Sassoulas left a comment


		# Adding (ignored) messages to the Message Reporter

		def _get_message_state_scope(

Move message adding functions from MessagesHandlerMixIn #5129

Move message adding functions from MessagesHandlerMixIn #5129

Conversation

DanielNoord commented Oct 7, 2021 • edited Loading

Type of Changes

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 7, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1317624768

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Move message adding functions from `MessagesHandlerMixIn` #5129

Move message adding functions from `MessagesHandlerMixIn` #5129

DanielNoord commented Oct 7, 2021 •

edited

Loading

Pierre-Sassoulas Oct 9, 2021 •

edited

Loading

Pierre-Sassoulas Oct 10, 2021 •

edited

Loading

coveralls commented Oct 7, 2021 •

edited

Loading