Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and typing of OutputLine #5060

Merged
merged 7 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 39 additions & 22 deletions pylint/testutils/output_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE

import collections
from typing import Any, NamedTuple
from typing import Any, Iterable, List, NamedTuple, Optional, Tuple, Union

from astroid import nodes

from pylint import interfaces
from pylint.constants import PY38_PLUS
from pylint.interfaces import HIGH, UNDEFINED, Confidence
from pylint.message.message import Message
from pylint.testutils.constants import UPDATE_OPTION


Expand All @@ -16,19 +19,30 @@ class MessageTest(
):
"""Used to test messages produced by pylint. Class name cannot start with Test as pytest doesn't allow constructors in test classes."""

def __new__(cls, msg_id, line=None, node=None, args=None, confidence=None):
def __new__(
cls,
msg_id: str,
line: Optional[int] = None,
node: Optional[nodes.NodeNG] = None,
args: Any = None,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

confidence: Optional[Confidence] = None,
) -> "MessageTest":
Comment on lines +22 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return tuple.__new__(cls, (msg_id, line, node, args, confidence))

def __eq__(self, other):
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])
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +32 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return NotImplemented # pragma: no cover


class MalformedOutputLineException(Exception):
def __init__(self, row, exception):
def __init__(
self,
row: Union[Tuple[str, ...], List[str], str],
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
exception: Exception,
) -> None:
example = "msg-symbolic-name:42:27:MyClass.my_function:The message"
other_example = "msg-symbolic-name:7:42::The message"
expected = [
Expand Down Expand Up @@ -62,39 +76,42 @@ def __init__(self, row, exception):
class OutputLine(NamedTuple):
symbol: str
lineno: int
column: str
object: Any
column: int
object: str
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

msg: str
confidence: interfaces.Confidence
confidence: str

@classmethod
def from_msg(cls, msg):
def from_msg(cls, msg: Message) -> "OutputLine":
column = cls.get_column(msg.column)
return cls(
msg.symbol,
msg.line,
column,
msg.obj or "",
msg.msg.replace("\r\n", "\n"),
msg.confidence.name
if msg.confidence != interfaces.UNDEFINED
else interfaces.HIGH.name,
msg.confidence.name if msg.confidence != UNDEFINED else HIGH.name,
)

@classmethod
def get_column(cls, column):
if not PY38_PLUS:
return "" # pragma: no cover
return str(column)
def get_column(cls, column: str) -> int:
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
if (
not PY38_PLUS
): # We check the column only for the new better ast parser introduced in python 3.8
return 0 # pragma: no cover
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
return int(column)

@classmethod
def from_csv(cls, row):
def from_csv(cls, row: Union[Tuple[str, ...], List[str], str]) -> "OutputLine":
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
try:
confidence = row[5] if len(row) == 6 else interfaces.HIGH.name
column = cls.get_column(row[2])
return cls(row[0], int(row[1]), column, row[3], row[4], confidence)
if isinstance(row, Iterable) and len(row) == 6:
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

except Exception as e:
raise MalformedOutputLineException(row, e) from e

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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

27 changes: 14 additions & 13 deletions tests/testutils/test_output_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@

# pylint: disable=redefined-outer-name

from typing import Callable, List, Optional
from typing import Callable, Optional

import pytest

from pylint.constants import PY38_PLUS
from pylint.interfaces import HIGH, INFERENCE, Confidence
from pylint.message import Message
from pylint.testutils.output_line import MalformedOutputLineException, OutputLine
from pylint.typing import MessageLocationTuple


@pytest.fixture()
Expand All @@ -19,14 +20,14 @@ def inner(confidence: Confidence = HIGH) -> Message:
return Message(
symbol="missing-docstring",
msg_id="C0123",
location=[ # type: ignore
location=MessageLocationTuple(
"abspath",
"path",
"module",
"obj",
"line",
"column",
],
1,
2,
),
msg="msg",
confidence=confidence,
)
Expand All @@ -37,11 +38,11 @@ def inner(confidence: Confidence = HIGH) -> Message:
def test_output_line() -> None:
output_line = OutputLine(
symbol="missing-docstring",
lineno=0,
column="0",
lineno=1,
column=2,
object="",
msg="Missing docstring's bad.",
confidence=HIGH,
confidence=HIGH.name,
)
assert output_line.symbol == "missing-docstring"

Expand All @@ -56,10 +57,10 @@ def test_output_line_from_message(message: Callable) -> None:
def test_output_line_to_csv(confidence: Confidence, message: Callable) -> None:
output_line = OutputLine.from_msg(message(confidence))
csv = output_line.to_csv()
expected_column = "column" if PY38_PLUS else ""
expected_column = "2" if PY38_PLUS else "0"
assert csv == (
"missing-docstring",
"line",
"1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expected_column,
"obj",
"msg",
Expand All @@ -84,10 +85,10 @@ def test_output_line_from_csv_error() -> None:
"confidence,expected_confidence", [[None, "HIGH"], ["INFERENCE", "INFERENCE"]]
)
def test_output_line_from_csv(
confidence: Optional[str], expected_confidence: Confidence
confidence: Optional[str], expected_confidence: str
) -> None:
if confidence:
proper_csv: List[str] = [
proper_csv = [
"missing-docstring",
"1",
"2",
Expand All @@ -98,7 +99,7 @@ def test_output_line_from_csv(
else:
proper_csv = ["missing-docstring", "1", "2", "obj", "msg"]
output_line = OutputLine.from_csv(proper_csv)
expected_column = "2" if PY38_PLUS else ""
expected_column = 2 if PY38_PLUS else 0
assert output_line == OutputLine(
symbol="missing-docstring",
lineno=1,
Expand Down