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

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature
📜 Docs

Description

Blocked by #5343
Ref #5336

This updates the reporters of pylint to start using end_line and end_column. It is the first PR as mentioned in #5343 (comment)

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Blocked 🚧 Blocked by a particular issue Documentation 📗 labels Nov 22, 2021
@DanielNoord DanielNoord added this to the 2.12.0 milestone Nov 22, 2021
pylint/reporters/text.py Outdated Show resolved Hide resolved
@DanielNoord DanielNoord marked this pull request as ready for review November 22, 2021 22:11
@coveralls
Copy link

coveralls commented Nov 22, 2021

Pull Request Test Coverage Report for Build 1499421022

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 93.483%

Totals Coverage Status
Change from base Build 1499266391: 0.007%
Covered Lines: 13972
Relevant Lines: 14946

💛 - Coveralls

ChangeLog Outdated Show resolved Hide resolved
doc/user_guide/output.rst Outdated Show resolved Hide resolved
doc/whatsnew/2.12.rst Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
Comment on lines 198 to 202
for key, value in self_dict.items():
# pylint: disable=fixme
# TODO: Add column to list of attributes to be printed as an empty string
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!

# TODO: Add column to list of attributes to be printed as an empty string
if value is None and key in PRINT_AS_EMPTY_STRING:
self_dict[key] = ""
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.

pylint/reporters/text.py Outdated Show resolved Hide resolved
DanielNoord and others added 2 commits November 22, 2021 23:19
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
# TODO: Add column to list of attributes to be printed as an empty string
if value is None and key in PRINT_AS_EMPTY_STRING:
self_dict[key] = ""
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.

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.

Comment on lines 198 to 206
# Check to see if all parameters in the template are attributes of the Message
if not self._checked_template:
template = self._template
parameters = re.findall(r"\{(.+?)\}", template)
for parameter in parameters:
if parameter not in self_dict:
template = template.replace(f"{{{parameter}}}", "")
self._template = template
self._checked_template = True
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved to on_set_current_module or even __init__? Don't now if the config settings are defined at the point already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to on_set_current_module

# Check to see if all parameters in the template are attributes of the Message
if not self._checked_template:
template = self._template
parameters = re.findall(r"\{(.+?)\}", template)
Copy link
Member

Choose a reason for hiding this comment

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

The regex doesn't quiet work with formatting strings. Maybe something like this?

{([^:]+?)(?::([^:]+))?}

for

"{line:03d}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used a new pattern. I think my new one works as well although the replacing is quite ugly.

parameters = re.findall(r"\{(.+?)\}", template)
for parameter in parameters:
if parameter not in self_dict:
template = template.replace(f"{{{parameter}}}", "")
Copy link
Member

Choose a reason for hiding this comment

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

Is it save to overwrite self._template directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be in its new place

@DanielNoord DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Nov 23, 2021
Comment on lines 191 to 197
# 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!

@DanielNoord DanielNoord requested a review from cdce8p November 23, 2021 08:17
tests/unittest_reporting.py Outdated Show resolved Hide resolved
tests/unittest_reporting.py Outdated Show resolved Hide resolved
DanielNoord and others added 2 commits November 23, 2021 09:58
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
pylint/reporters/text.py Outdated Show resolved Hide resolved
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``.
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

@Pierre-Sassoulas Pierre-Sassoulas dismissed their stale review November 23, 2021 19:23

Did not see change to JsonReporter

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Nov 24, 2021

@DanielNoord I fixed the merged conflict but is this ready to review ? I'm a little lost about the order to follow between #5349 #5372 and #5336

@DanielNoord
Copy link
Collaborator Author

@DanielNoord I fixed the merged conflict but is this ready to review ? I'm a little lost about the order to follow between #5349 #5372 and #5336

I think you mean #5349, #5372 and #5376.

They are already for review and can be merged separately. They don't depend on each other!

@Pierre-Sassoulas
Copy link
Member

#5376 was not in the 2.12.0 milestone I added it.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM ! I only have a small question.

* 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!

@Pierre-Sassoulas Pierre-Sassoulas merged commit 7c3533c into pylint-dev:main Nov 24, 2021
@DanielNoord DanielNoord deleted the column-reporter branch November 24, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants