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

Update reporters to (allow) use of end_line and end_column #5372

Merged
merged 17 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ Release date: TBA

Closes #4982

* Add ability to add ``end_line`` and ``end_column`` to the ``--msg-template`` option.
With the standard ``TextReporter`` this will add the line and column number of the
end of a node to the output of Pylint. If these numbers are unknown they are represented
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
by an empty string.

* Add ``end_line`` and ``end_column`` fields to the output of the ``JSONReporter``.

* Fix ``install graphiz`` message which isn't needed for puml output format.

* Fix ``simplify-boolean-expression`` when condition can be inferred as False.
Expand Down
7 changes: 7 additions & 0 deletions doc/user_guide/output.rst
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ line
line number
column
column number
end_line
line number of the end of the node
end_column
column number of the end of the node
module
module name
obj
Expand Down Expand Up @@ -94,6 +98,9 @@ A few other examples:
The ``--msg-template`` option can only be combined with text-based reporters (``--output-format`` either unspecified or one of: parseable, colorized or msvs).
If both ``--output-format`` and ``--msg-template`` are specified, the ``--msg-template`` option will take precedence over the default line format defined by the reporter class.

If ``end_line`` or ``end_column`` are ``None`` they will be represented as an empty string
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
by the formatter.

.. _Python new format syntax: https://docs.python.org/2/library/string.html#formatstrings

Source code analysis section
Expand Down
7 changes: 7 additions & 0 deletions doc/whatsnew/2.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -198,3 +198,10 @@ Other Changes
* Fix crash on ``open()`` calls when the ``mode`` argument is not a simple string.

Partially closes #5321

* Add ability to add ``end_line`` and ``end_column`` to the ``--msg-template`` option.
With the standard ``TextReporter`` this will add the line and column number of the
end of a node to the output of Pylint. If these numbers are unknown they are represented
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
by an empty string.
Copy link
Member

Choose a reason for hiding this comment

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

I think we could also change the default for msg-template in 3.0. Do we want the default to have end line and end column ? This seems like something that is useful in an IDE but not a lot for a text output read by humans.

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 Argued that we might leave the default output as is because of the potential to break many tools. I think this is a discussion which (if we want to have it anyway) we could leave until we are closer to the release of 3.0. With this merge everybody that wants to can use this new feature!


* Add ``end_line`` and ``end_column`` fields to the output of the ``JSONReporter``.
Copy link
Member

Choose a reason for hiding this comment

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

It look like a breaking change that should wait for #4741. We have a 3.0 alpha branch, might be the time to dust it off ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted! See #5380

2 changes: 2 additions & 0 deletions pylint/reporters/json_reporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ def display_messages(self, layout: Optional["Section"]) -> None:
"obj": msg.obj,
"line": msg.line,
"column": msg.column,
"end_line": msg.end_line,
"end_column": msg.end_column,
"path": msg.path,
"symbol": msg.symbol,
"message": msg.msg or "",
Expand Down
13 changes: 12 additions & 1 deletion pylint/reporters/text.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ class MessageStyle(NamedTuple):
"cyan": "36",
"white": "37",
}
PRINT_AS_EMPTY_STRING = {"end_line", "end_column"}
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
"""Set of Message attributes that should be printed as an
empty string when they are None"""


def _get_ansi_code(msg_style: MessageStyle) -> str:
Expand Down Expand Up @@ -177,6 +180,8 @@ class TextReporter(BaseReporter):
__implements__ = IReporter
name = "text"
extension = "txt"
# pylint: disable=fixme
# TODO: Add end_line and end_column to standard line format of TextReporter
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
line_format = "{path}:{line}:{column}: {msg_id}: {msg} ({symbol})"

def __init__(self, output: Optional[TextIO] = None) -> None:
Expand All @@ -189,7 +194,13 @@ def on_set_current_module(self, module: str, filepath: Optional[str]) -> None:

def write_message(self, msg: Message) -> None:
"""Convenience method to write a formatted message with class default template"""
self.writeln(msg.format(self._template))
self_dict = msg._asdict()
for key, value in self_dict.items():
# pylint: disable=fixme
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
# TODO: Add column to list of attributes to be printed as an empty string
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
if value is None and key in PRINT_AS_EMPTY_STRING:
self_dict[key] = ""
Copy link
Member

Choose a reason for hiding this comment

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

Is a for - loop really necessary here? Maybe iterate only over PRINT_AS_EMPTY_STRING and do

self_dict[key] = self_dict[key] or ""

Copy link
Member

Choose a reason for hiding this comment

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

Or even better, what about this?

Initialize end_lineno and end_col_offset with an empty string instead "". You still would be able to differentiate between a null value and a number. I.e. int | Literal[""] as type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or even better, what about this?

Initialize end_lineno and end_col_offset with an empty string instead "". You still would be able to differentiate between a null value and a number. I.e. int | Literal[""] as type.

I don't really like that as then we we're changing the type of end_lineno and end_col_offset while still "internal" to fit a specific type of reporter. For example, for the JSONReporter it makes no sense to have it be "". I'm not sure if other developer have written other Reporter but I think keeping both attributes None until a specific reporter starts handling them is the better option here.
I'll add your first suggestion!

self.writeln(self._template.format(**self_dict))
Copy link
Member

Choose a reason for hiding this comment

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

self.writeln(msg.format(self._template)) has been quite elegant, but there is one drawback. Calling it with an unknown key raises a KeyError. Ideally we could check a template and replace invalid values with "" beforehand. I'm currently having the issue to implement some kind of version check in vscode-python as calling pylint with the new msg-template would raise this error.

Traceback (most recent call last):
  File "/.../venv-310/bin/pylint", line 33, in <module>
    sys.exit(load_entry_point('pylint', 'console_scripts', 'pylint')())
  File "/.../pylint/__init__.py", line 24, in run_pylint
    PylintRun(sys.argv[1:])
  File "/.../pylint/lint/run.py", line 398, in __init__
    linter.check(args)
  File "/.../pylint/lint/pylinter.py", line 992, in check
    self._check_files(
  File "/.../pylint/lint/pylinter.py", line 1041, in _check_files
    self.add_message(symbol, args=msg)
  File "/.../pylint/lint/pylinter.py", line 1513, in add_message
    self._add_one_message(
  File "/.../pylint/lint/pylinter.py", line 1471, in _add_one_message
    self.reporter.handle_message(
  File "/.../pylint/reporters/text.py", line 202, in handle_message
    self.write_message(msg)
  File "/.../pylint/reporters/text.py", line 192, in write_message
    self.writeln(msg.format(self._template))
  File "/.../pylint/message/message.py", line 94, in format
    return template.format(**self._asdict())
KeyError: 'msg1'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't that just raise a new warning? Something like config-parse-error as introduced in #5365. We could emit it on an except for the KeyError.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't really solve the issue. At the core of it its backwards incompatibility.

We should be able to use a new template, i.e. with end_lineno, in old pylint versions and still get a usable result. The way it's now, I need to make sure only to add end_lineno if we are using the new pylint version.

If at some point we add another key, this will only repeat. Every time incompatible with old versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Came up with something. A little hacky, but let me know what you think. I added if not self._checked_template: because I thought the additional if statement would be better than doing the regex statement each line.


def handle_message(self, msg: Message) -> None:
"""manage message of different type and in the context of path"""
Expand Down
80 changes: 63 additions & 17 deletions tests/unittest_reporters_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,29 +25,69 @@
from pylint.reporters.ureports.nodes import EvaluationSection

expected_score_message = "Expected score message"
expected_result = [
[
("column", 0),
("line", 1),
("message", "Line too long (1/2)"),
("message-id", "C0301"),
("module", "0123"),
("obj", ""),
("path", "0123"),
("symbol", "line-too-long"),
("type", "convention"),
]
]


def test_simple_json_output_no_score() -> None:
report = get_linter_result(score=False)
"""Test JSON reporter with no score"""
message = {
"msg": "line-too-long",
"line": 1,
"args": (1, 2),
"end_line": None,
"end_column": None,
}
expected = [
[
("column", 0),
("line", 1),
("end_line", None),
("end_column", None),
("message", "Line too long (1/2)"),
("message-id", "C0301"),
("module", "0123"),
("obj", ""),
("path", "0123"),
("symbol", "line-too-long"),
("type", "convention"),
]
]
report = get_linter_result(score=False, message=message)
assert len(report) == 1
report_result = [sorted(report[0].items(), key=lambda item: item[0])]
assert report_result == expected


def test_simple_json_output_no_score_with_end_line() -> None:
"""Test JSON reporter with no score with end_line and end_column"""
message = {
"msg": "line-too-long",
"line": 1,
"args": (1, 2),
"end_line": 1,
"end_column": 4,
}
expected = [
[
("column", 0),
("line", 1),
("end_line", 1),
("end_column", 4),
("message", "Line too long (1/2)"),
("message-id", "C0301"),
("module", "0123"),
("obj", ""),
("path", "0123"),
("symbol", "line-too-long"),
("type", "convention"),
]
]
report = get_linter_result(score=False, message=message)
assert len(report) == 1
report_result = [sorted(report[0].items(), key=lambda item: item[0])]
assert report_result == expected_result
assert report_result == expected


def get_linter_result(score: bool) -> List[Dict[str, Any]]:
def get_linter_result(score: bool, message: Dict[str, Any]) -> List[Dict[str, Any]]:
output = StringIO()
reporter = JSONReporter(output)
linter = PyLinter(reporter=reporter)
Expand All @@ -56,7 +96,13 @@ def get_linter_result(score: bool) -> List[Dict[str, Any]]:
linter.config.score = score
linter.open()
linter.set_current_module("0123")
linter.add_message("line-too-long", line=1, args=(1, 2))
linter.add_message(
message["msg"],
line=message["line"],
args=message["args"],
end_lineno=message["end_line"],
end_col_offset=message["end_column"],
)
# we call those methods because we didn't actually run the checkers
if score:
reporter.display_reports(EvaluationSection(expected_score_message))
Expand Down
34 changes: 34 additions & 0 deletions tests/unittest_reporting.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,40 @@ def test_template_option(linter):
assert output.getvalue() == "************* Module 0123\nC0301:001\nC0301:002\n"


def test_template_option_default(linter) -> None:
"""Test the default msg-template setting"""
output = StringIO()
linter.reporter.out = output
linter.open()
linter.set_current_module("my_module")
linter.add_message("C0301", line=1, args=(1, 2))
linter.add_message("line-too-long", line=2, args=(3, 4))

out_lines = output.getvalue().split("\n")
assert out_lines[1] == "my_module:1:0: C0301: Line too long (1/2) (line-too-long)"
assert out_lines[2] == "my_module:2:0: C0301: Line too long (3/4) (line-too-long)"


def test_template_option_end_line(linter) -> None:
"""Test the msg-template option with end_line and end_column"""
output = StringIO()
linter.reporter.out = output
linter.set_option(
"msg-template",
"{path}:{line}:{column}:{end_line}:{end_column}: {msg_id}: {msg} ({symbol})",
)
linter.open()
linter.set_current_module("my_mod")
linter.add_message("C0301", line=1, args=(1, 2))
linter.add_message(
"line-too-long", line=2, end_lineno=2, end_col_offset=4, args=(3, 4)
)

out_lines = output.getvalue().split("\n")
assert out_lines[1] == "my_mod:1:0::: C0301: Line too long (1/2) (line-too-long)"
assert out_lines[2] == "my_mod:2:0:2:4: C0301: Line too long (3/4) (line-too-long)"


def test_deprecation_set_output(recwarn):
"""TODO remove in 3.0""" # pylint: disable=fixme
reporter = BaseReporter()
Expand Down