Refactor and typing of OutputLine #5060

DanielNoord · 2021-09-21T15:15:10Z

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

This brings OutputLine in line with the classes dealing with Message by making the line and column attributes int. This has increased the size of the diff but I felt that making line and column int throughout the codebase would eventually be worthwhile. OutputLine in csv form still only consists of str (in fact we now make sure this is the case) but whenever we deal with it "internally" we can now assume line and column are int.
The tests have also been changed to accommodate for this.

DanielNoord · 2021-09-21T15:15:59Z

pylint/testutils/output_line.py

+        msg_id: str,
+        line: Optional[int] = None,
+        node: Optional[nodes.NodeNG] = None,
+        args: Any = None,


Similar to what we did for Message in #5050. This should at some point be fully investigated and changed.

DanielNoord · 2021-09-21T15:16:45Z

pylint/testutils/output_line.py

        if isinstance(other, MessageTest):
            if self.confidence and other.confidence:
                return super().__eq__(other)
-            return self[:-1] == other[:-1]
+            return tuple(self[:-1]) == tuple(other[:-1])


mypy complains that on this line the self to __eq__ is no longer a MessageTest. Making both sides a tuple solves this issue.

DanielNoord · 2021-09-21T15:18:00Z

pylint/testutils/output_line.py

-    column: str
-    object: Any
+    column: int
+    object: str


I have checked the full test suite by printing object in OutputLine.from_msg(). This was always str, which is to be expected since we are dealing with csv and output lines here.

pylint/testutils/output_line.py

DanielNoord · 2021-09-21T15:20:52Z

pylint/testutils/output_line.py

-    def to_csv(self):
-        return tuple(self)
+    def to_csv(self) -> Tuple[str, str, str, str, str, str]:
+        return tuple(str(i) for i in self)  # type: ignore # pylint: disable=not-an-iterable


pylint finds that self is non-iterable, although I think we can assume it is, right?

Furthermore, mypy sees the return value as being Tuple[str, ...]. I added checks with a temp value and isinstance(temp_csv, Tuple) and len(temp_csv) == 6, but it kept denying the correct size. Eventually I opted to add an ignore.

@cdce8p btw, this is a false positive right? Or am I missing some edge case where this is indeed a valid error?

DanielNoord · 2021-09-21T15:21:44Z

tests/testutils/test_output_line.py

    assert csv == (
        "missing-docstring",
-        "line",
+        "1",


Note that we are checking the to_csv() method here, so line and column should be str again.

Pierre-Sassoulas · 2021-09-21T15:19:10Z

pylint/testutils/output_line.py

+            if isinstance(row, Iterable) and len(row) == 6:
+                return cls(row[0], int(row[1]), column, row[3], row[4], row[5])
+            if isinstance(row, Iterable) and len(row) == 5:
+                return cls(row[0], int(row[1]), column, row[3], row[4], HIGH.name)
+            raise IndexError


Maybe we could uniformize the expected text output by adding the confidence everywhere, so there's only one type of output and we can treat all expected message the same ? (This would prevent big diff where we add the confidence after upgrading the expected output). @cdce8p what do you think, I remember that we had this conversation already at some point.

This should definitely be part of a separate PR and would then ideally follow after this. If desired I can go and create this. Should be as easy as running python tests/test_functional.py --update-functional-output and updating the typing and this function here.

Just my personal opinion: I don't see much value in adding HIGH to almost every pylint error in all functional tests. Especially since it's the last value, it would be much more useful to only specify it if it's different (i.e. not HIGH).

While I agree with the no added value argument, I think Pierre makes a valid point that when we do this once we will then have less larger diffs after people use the --update-functional-output to update tests after they have added new cases. I saw this during my own work as well, where all test data gets updated because of it.
I don't think it would hurt to do this once and would allow simplifying the code here.

We can update / revert --update-functional-output to only add different confidence values 😜 Then we would only need to update a few.

Anyway, doesn't matter the decision, this should be part of another PR. (I saw a question about that earlier.)

coveralls · 2021-09-22T10:16:42Z

Pull Request Test Coverage Report for Build 1263765687

26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.003%) to 93.089%

Totals
Change from base Build 1257471391:	0.003%
Covered Lines:	13348
Relevant Lines:	14339

💛 - Coveralls

cdce8p · 2021-09-22T21:48:47Z

pylint/testutils/output_line.py

+    def __new__(
+        cls,
+        msg_id: str,
+        line: Optional[int] = None,
+        node: Optional[nodes.NodeNG] = None,
+        args: Any = None,
+        confidence: Optional[Confidence] = None,
+    ) -> "MessageTest":


Ideally we would use typing.NamedTuple with default values instead. However that requires Python 3.6.1 -> see #5065

Do you want to block this PR before consensus is reached there?

Don't think that would make sense here. Even if we reach a decision, the version bump would likely be in 2.12 at the earliest.

cdce8p · 2021-09-22T21:49:47Z

pylint/testutils/output_line.py

+    def __eq__(self, other: object) -> bool:
        if isinstance(other, MessageTest):
            if self.confidence and other.confidence:
                return super().__eq__(other)
-            return self[:-1] == other[:-1]
+            return tuple(self[:-1]) == tuple(other[:-1])


This would be an even better candidate for a dataclass. That however requires Python 3.7 😞

pylint/testutils/output_line.py

cdce8p · 2021-09-22T22:01:37Z

pylint/testutils/output_line.py

+            if isinstance(row, Iterable) and len(row) == 6:
+                return cls(row[0], int(row[1]), column, row[3], row[4], row[5])
+            if isinstance(row, Iterable) and len(row) == 5:
+                return cls(row[0], int(row[1]), column, row[3], row[4], HIGH.name)
+            raise IndexError


Just my personal opinion: I don't see much value in adding HIGH to almost every pylint error in all functional tests. Especially since it's the last value, it would be much more useful to only specify it if it's different (i.e. not HIGH).

pylint/testutils/output_line.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

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

pylint/testutils/output_line.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p

Last comment from me. I'll leave the rest to you @Pierre-Sassoulas

pylint/testutils/output_line.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

Pierre-Sassoulas

👍

Refactor and typing of OutputLine

6235008

DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 21, 2021

DanielNoord commented Sep 21, 2021

View reviewed changes

Pierre-Sassoulas reviewed Sep 21, 2021

View reviewed changes

Update pylint/testutils/output_line.py

c467240

cdce8p reviewed Sep 22, 2021

View reviewed changes

DanielNoord and others added 3 commits September 23, 2021 00:15

Apply suggestions from code review

dbd47f9

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

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

2a97b30

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

Code review

136d3cf

cdce8p reviewed Sep 22, 2021

View reviewed changes

pylint/testutils/output_line.py Outdated Show resolved Hide resolved

Update pylint/testutils/output_line.py

dc188a9

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p reviewed Sep 22, 2021

View reviewed changes

pylint/testutils/output_line.py Outdated Show resolved Hide resolved

Update pylint/testutils/output_line.py

9942f56

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p mentioned this pull request Sep 23, 2021

Require Python 3.6.2 #5068

Merged

DanielNoord requested a review from Pierre-Sassoulas September 23, 2021 12:41

Pierre-Sassoulas approved these changes Sep 23, 2021

View reviewed changes

DanielNoord merged commit 5124f54 into pylint-dev:main Sep 23, 2021

DanielNoord deleted the typing-outputline branch September 23, 2021 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and typing of OutputLine #5060

Refactor and typing of OutputLine #5060

DanielNoord commented Sep 21, 2021

DanielNoord Sep 21, 2021

DanielNoord Sep 21, 2021

DanielNoord Sep 21, 2021

DanielNoord Sep 21, 2021

DanielNoord Sep 23, 2021

DanielNoord Sep 21, 2021

Pierre-Sassoulas Sep 21, 2021

DanielNoord Sep 22, 2021

cdce8p Sep 22, 2021

DanielNoord Sep 22, 2021

cdce8p Sep 22, 2021

coveralls commented Sep 22, 2021 •

edited

Loading

cdce8p Sep 22, 2021

DanielNoord Sep 22, 2021

cdce8p Sep 22, 2021

cdce8p Sep 22, 2021

cdce8p Sep 22, 2021

cdce8p left a comment

Pierre-Sassoulas left a comment

Refactor and typing of OutputLine #5060

Refactor and typing of OutputLine #5060

Conversation

DanielNoord commented Sep 21, 2021

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

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 Sep 22, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1263765687

💛 - Coveralls

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

cdce8p left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

coveralls commented Sep 22, 2021 •

edited

Loading