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.

Sign up for GitHub

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

Jump to bottom

Fix disabling of fixme #7144

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Fix disabling of fixme #7144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b93bffe
Add ``fixme`` documentation examples
DanielNoord Jul 7, 2022
9998154
Fix disabling of ``fixme``
DanielNoord Jul 7, 2022
0f68480
Update tests/functional/f/fixme.py
DanielNoord Jul 7, 2022
813f5ca
Update doc/whatsnew/2/2.14/full.rst
DanielNoord Jul 7, 2022
714c4c4
Add details
DanielNoord Jul 7, 2022
31bcdd6
Merge branch 'fixme' of https://github.com/DanielNoord/pylint into fixme
DanielNoord Jul 7, 2022
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
5 changes: 5 additions & 0 deletions doc/data/messages/f/fixme/bad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# FIXME: Create an issue on the bug tracker for this refactor we might do someday # [fixme]

...

# TODO: We should also fix this at some point # [fixme]
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion doc/data/messages/f/fixme/details.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
You can help us make the doc better `by contributing <https://github.com/PyCQA/pylint/issues/5953>`_ !
Copy link
Member

Choose a reason for hiding this comment

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

It's to get crafty with regex to force the presence of a ticket number, for example see #2874 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I'm suggesting to add that text in detail.rst, but it's impossible to suggest on remove file :)

(My pylint dev machine just died, I lost a lot of unpushed branch :( )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh not. That sounds bad... Might be able to recover some from your fork?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah some branches I pushed semi regularly, everything experimental or unfinished though... (especially a massive invalid-name refactor for name that were too small...) Well at least I won't have to clean up the git repo anymore and I get to start from a clean slate 🥲 I had a disk space problem for some time, and after months of just deleting the cache from time to time I think I finally had something important that did not get written correctly (auth app crashed) 😅

You can get use regular expressions and the ``notes-rgx`` option to create some constraints for this message.
See `the following issue <https://github.com/PyCQA/pylint/issues/2874>`_ for some examples.
6 changes: 5 additions & 1 deletion doc/data/messages/f/fixme/good.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
# This is a placeholder for correct code for this message.
# I no longer want to fix this

...

# I have fixed the issue
3 changes: 3 additions & 0 deletions doc/whatsnew/2/2.14/full.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ Release date: TBA

Closes #7003

* Fixed the disabling of ``fixme`` and its interaction with ``useless-suppression``.


What's New in Pylint 2.14.4?
----------------------------
Release date: 2022-06-29
Expand Down
42 changes: 6 additions & 36 deletions pylint/checkers/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

from pylint.checkers import BaseRawFileChecker, BaseTokenChecker
from pylint.typing import ManagedMessage
from pylint.utils.pragma_parser import OPTION_PO, PragmaParserError, parse_pragma

if TYPE_CHECKING:
from pylint.lint import PyLinter
Expand Down Expand Up @@ -134,45 +133,16 @@ def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
"""Inspect the source to find fixme problems."""
if not self.linter.config.notes:
return
comments = (
token_info for token_info in tokens if token_info.type == tokenize.COMMENT
)
for comment in comments:
comment_text = comment.string[1:].lstrip() # trim '#' and white-spaces

# handle pylint disable clauses
disable_option_match = OPTION_PO.search(comment_text)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this code is doing well. We were ignoring fixme if there was something looking like a pylint disable comment ? 😕

i.e. pylint: disable=fixme TODO or pylint: disable=no-member TODO should not raise ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we were seeing if there is a # pylint: disable=fixme on the same line as the fixme comment. However, we have other code that does this for us and it doesn't allow using disable-next.
Just removing is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the part where it's specific to "fixme" though, wasn't it a special case where you could add TODO in pylint disable ? (Someone too lazy to do a grep for "pylint: disable" at some point, but not lazy enough to not add TODO everywhere ?)

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 don't think it actually does anything other than disallow disable. PragmaParserError probably catches all other issues.

The fact that all of our enable tests work without this shows that it is completely unnecessary 😄

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for asking but this cleanup looked too good to be true 😄 I'm just amazed at the number of time we just remove a bunch of code to fix a bug. It's been happening over and over.

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 also was quite surprised when I found this code. At first I though for some reason the fixme checker was responsible for parsing all pragma's but then I remembered that I actually refactored that code myself and made sure that it will always be PyLinter ... 😅

Copy link
Member

Choose a reason for hiding this comment

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

Having refactored half the codebase has its perks 😄

if disable_option_match:
try:
values = []
try:
for pragma_repr in (
p_rep
for p_rep in parse_pragma(disable_option_match.group(2))
if p_rep.action == "disable"
):
values.extend(pragma_repr.messages)
except PragmaParserError:
# Printing useful information dealing with this error is done in the lint package
pass
except ValueError:
self.add_message(
"bad-inline-option",
args=disable_option_match.group(1).strip(),
line=comment.start[0],
)
continue
self.linter.add_ignored_message("fixme", line=comment.start[0])
for token_info in tokens:
if token_info.type != tokenize.COMMENT:
continue

# emit warnings if necessary
match = self._fixme_pattern.search("#" + comment_text.lower())
if match:
comment_text = token_info.string[1:].lstrip() # trim '#' and white-spaces
if self._fixme_pattern.search("#" + comment_text.lower()):
self.add_message(
"fixme",
col_offset=comment.start[1] + 1,
col_offset=token_info.start[1] + 1,
args=comment_text,
line=comment.start[0],
line=token_info.start[0],
)


Expand Down
14 changes: 11 additions & 3 deletions tests/functional/f/fixme.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- encoding=utf-8 -*-
# pylint: disable=missing-docstring, unused-variable
"""Tests for fixme and its disabling and enabling."""
# pylint: disable=missing-function-docstring, unused-variable

# +1: [fixme]
# FIXME: beep
Expand Down Expand Up @@ -29,5 +29,13 @@ def function():

#FIXME: in fact nothing to fix #pylint: disable=fixme
#TODO: in fact nothing to do #pylint: disable=fixme
#TODO: in fact nothing to do #pylint: disable=line-too-long, fixme
#TODO: in fact nothing to do #pylint: disable=line-too-long, fixme, useless-suppression
# Todoist API mentioned should not result in a message.

# pylint: disable-next=fixme
# FIXME: Don't raise when the message is disabled

# This line needs to be at the end of the file to make sure it doesn't end with a comment
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
# Pragma's compare against the 'lineno' attribute of the respective nodes which
# would stop too soon otherwise.
print()
1 change: 1 addition & 0 deletions tests/functional/f/fixme.rc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
notes=XXX,TODO,./TODO
# Regular expression of note tags to take in consideration.
notes-rgx=FIXME(?!.*ISSUE-\d+)|TO.*DO
enable=useless-suppression