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

Fixme improvements #2320

Merged
merged 5 commits into from
Jul 26, 2018
Merged

Fixme improvements #2320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea5e273
Simplify fixme regex
scop Jul 18, 2018
d254ccf
Require word boundary after fixme notes
scop Jul 18, 2018
3e77fea
Escape regex metachars in configured fixme note tags
scop Jul 18, 2018
4191af3
Add my copyright note
scop Jul 18, 2018
82e2e2c
Drop simple "in" matching optimization from fixme
scop Jul 24, 2018
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
14 changes: 2 additions & 12 deletions pylint/checkers/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# Copyright (c) 2016 glegoux <[email protected]>
# Copyright (c) 2017-2018 hippo91 <[email protected]>
# Copyright (c) 2017 Mikhail Fesenko <[email protected]>
# Copyright (c) 2018 Ville Skyttä <[email protected]>

# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/master/COPYING
Expand Down Expand Up @@ -97,17 +98,6 @@ def _check_note(self, notes, lineno, line, module_last_lineno):
commented lines exist at the end of the module)
:type module_last_lineno: int
"""
# First, simply check if the notes are in the line at all. This is an
# optimisation to prevent using the regular expression on every line,
# but rather only on lines which may actually contain one of the notes.
# This prevents a pathological problem with lines that are hundreds
# of thousands of characters long.
for note in map(str.lower, self.config.notes):
if note in line.lower():
break
else:
return

match = notes.search(line)
if not match:
return
Expand Down Expand Up @@ -151,7 +141,7 @@ def process_module(self, module):
"""
if self.config.notes:
notes = re.compile(
r'.*?#\s*(%s)(:*\s*.*)' % "|".join(self.config.notes), re.I)
r'#\s*(%s)\b' % "|".join(map(re.escape, self.config.notes)), re.I)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure the word boundary is needed? The new expression is not able to catch anything and right now it passes because we also do an in check earlier at line 106, before actually searching with the expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The word boundary is needed, so that a warning doesn't get emitted about e.g. # Todoist API. The commits contain test cases which should cause failures if the changes don't work, and they did fail when I was making these changes, so at least the tests worked back then. But now they pass (also in Travis), so I'm not sure what to say :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a couple of manual tests:

>>> import re
>>> myre = re.compile(r'#\s*(%s)\b' % "|".join(map(re.escape, ['todo', 'fixme'])), re.I)
>>> myre.search("# TODO foo")
<re.Match object; span=(0, 6), match='# TODO'>
>>> myre.search("# Todoist")
>>>

At least these work as intended: First matches, second doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

@scop I probably forgot to make the string a raw string. Also the test is passing because we do an in early on before applying the regular expression (https://github.com/PyCQA/pylint/pull/2320/files#diff-a4b1cfd0674823eef01c61047e6e6bdaL106). I think we should remove that and allow just the regex instead as the performance optimisation feels premature in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested locally (+ in Travis in my own account) and as I suspected, the in does not alter the outcome of the checks, they pass whether it's there or not. But yeah, dropped because I couldn't find any effect from it in a quick test (see commit message below).

else:
notes = None
if module.file_encoding:
Expand Down
1 change: 1 addition & 0 deletions pylint/test/functional/fixme.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ 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
# Todoist API mentioned should not result in a message.