-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from 2 commits
6235008
c467240
dbd47f9
2a97b30
136d3cf
dc188a9
9942f56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
||
|
@@ -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, | ||
confidence: Optional[Confidence] = None, | ||
) -> "MessageTest": | ||
Comment on lines
+22
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we would use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to block this PR before consensus is reached there? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+32
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be an even better candidate for a |
||
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 = [ | ||
|
@@ -62,39 +76,42 @@ def __init__(self, row, exception): | |
class OutputLine(NamedTuple): | ||
symbol: str | ||
lineno: int | ||
column: str | ||
object: Any | ||
column: int | ||
object: str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have checked the full test suite by printing object in |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just my personal opinion: I don't see much value in adding There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can update / revert 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Furthermore, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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, | ||
) | ||
|
@@ -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" | ||
|
||
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we are checking the |
||
expected_column, | ||
"obj", | ||
"msg", | ||
|
@@ -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", | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
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.