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 9 commits
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
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
by the default ``TextReporter``.

.. _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
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
15 changes: 14 additions & 1 deletion pylint/reporters/text.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
:colorized: an ANSI colorized text reporter
"""
import os
import re
import sys
import warnings
from typing import (
Expand Down Expand Up @@ -187,9 +188,21 @@ def __init__(self, output: Optional[TextIO] = None) -> None:
def on_set_current_module(self, module: str, filepath: Optional[str]) -> None:
self._template = str(self.linter.config.msg_template or self._template)

# Check to see if all parameters in the template are attributes of the Message
template = self._template
parameters = re.findall(r"\{(.+?)(:.*)?\}", template)
for parameter in parameters:
if parameter[0] not in Message._fields:
template = re.sub(r"\{" + parameter[0] + r"(:.*?)?\}", "", template)
self._template = template
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea, currently this is down for every module. Can we store the original template and the new one. Then for every subsequent call, compare the "new" self._template with the stored template and if they match use the fixed one.

Copy link
Member

Choose a reason for hiding this comment

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

It might also be a good idea to at least emit a short warning: Hey, we don't know this option are you sure?. Otherwise we fail silently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done and done!


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 in ("end_line", "end_column"):
self_dict[key] = self_dict[key] or ""

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),
("end_column", None),
("end_line", None),
("line", 1),
("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),
("end_column", 4),
("end_line", 1),
("line", 1),
("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
60 changes: 60 additions & 0 deletions tests/unittest_reporting.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,62 @@ 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_template_option_non_exisiting(linter) -> None:
"""Test the msg-template option with a non exisiting options.
This makes sure that this option remains backwards compatible as new
parameters do not break on previous versions"""
output = StringIO()
linter.reporter.out = output
linter.set_option(
"msg-template",
"{path}:{line}:{a_new_option}:({a_second_new_option:03d})",
)
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::()"
assert out_lines[2] == "my_mod:2::()"


def test_deprecation_set_output(recwarn):
"""TODO remove in 3.0""" # pylint: disable=fixme
reporter = BaseReporter()
Expand Down Expand Up @@ -153,6 +209,8 @@ def test_multi_format_output(tmp_path):
' "obj": "",\n'
' "line": 1,\n'
' "column": 0,\n'
' "end_line": null,\n'
' "end_column": null,\n'
f' "path": {escaped_source_file},\n'
' "symbol": "missing-module-docstring",\n'
' "message": "Missing module docstring",\n'
Expand All @@ -164,6 +222,8 @@ def test_multi_format_output(tmp_path):
' "obj": "",\n'
' "line": 1,\n'
' "column": 0,\n'
' "end_line": null,\n'
' "end_column": null,\n'
f' "path": {escaped_source_file},\n'
' "symbol": "line-too-long",\n'
' "message": "Line too long (1/2)",\n'
Expand Down