remove --diff option

This commit is contained in:
Anthony Sottile 2022-10-26 20:37:24 -07:00
parent 86268cbde8
commit fba6df88f9
18 changed files with 11 additions and 526 deletions

View file

@ -66,11 +66,3 @@ The standard library's :func:`fnmatch.fnmatch` is excellent at deciding if a
filename matches a single pattern. In our use case, however, we typically have
a list of patterns and want to know if the filename matches any of them. This
function abstracts that logic away with a little extra logic.
.. autofunction:: flake8.utils.parse_unified_diff
To handle usage of :option:`flake8 --diff`, |Flake8| needs to be able
to parse the name of the files in the diff as well as the ranges indicated the
sections that have been changed. This function either accepts the diff as an
argument or reads the diff from standard-in. It then returns a dictionary with
filenames as the keys and sets of line numbers as the value.

View file

@ -44,8 +44,6 @@ Index of Options
- :option:`flake8 --count`
- :option:`flake8 --diff`
- :option:`flake8 --exclude`
- :option:`flake8 --filename`
@ -230,27 +228,6 @@ Options and their Descriptions
count = True
.. option:: --diff
:ref:`Go back to index <top>`
.. warning::
Due to hiding potential errors, this option is deprecated and will be
removed in a future version.
Use the unified diff provided on standard in to only check the modified
files and report errors included in the diff.
Command-line example:
.. prompt:: bash
git diff -u | flake8 --diff
This **can not** be specified in config files.
.. option:: --exclude=<patterns>
:ref:`Go back to index <top>`

View file

@ -136,7 +136,6 @@ class StyleGuide:
stdin_display_name=self.options.stdin_display_name,
filename_patterns=self.options.filename,
exclude=self.options.exclude,
is_running_from_diff=self.options.diff,
)
)
return not paths

View file

@ -97,8 +97,6 @@ class Manager:
# implementation issues
# - the user provided stdin and that's not something we can handle
# well
# - we're processing a diff, which again does not work well with
# multiprocessing and which really shouldn't require multiprocessing
# - the user provided some awful input
# class state is only preserved when using the `fork` strategy.
@ -116,13 +114,6 @@ class Manager:
)
return 0
if self.options.diff:
LOG.warning(
"The --diff option was specified with --jobs but "
"they are not compatible. Ignoring --jobs arguments."
)
return 0
jobs = self.options.jobs
# If the value is "auto", we want to let the multiprocessing library
@ -169,7 +160,6 @@ class Manager:
stdin_display_name=self.options.stdin_display_name,
filename_patterns=self.options.filename,
exclude=self.exclude,
is_running_from_diff=self.options.diff,
)
]
self.checkers = [c for c in self._all_checkers if c.should_process]

View file

@ -55,7 +55,6 @@ def expand_paths(
stdin_display_name: str,
filename_patterns: Sequence[str],
exclude: Sequence[str],
is_running_from_diff: bool,
) -> Generator[str, None, None]:
"""Expand out ``paths`` from commandline to the lintable files."""
if not paths:
@ -75,24 +74,16 @@ def expand_paths(
logger=LOG,
)
def is_included(arg: str, fname: str) -> bool:
# while running from a diff, the arguments aren't _explicitly_
# listed so we still filter them
if is_running_from_diff:
return utils.fnmatch(fname, filename_patterns)
else:
return (
# always lint `-`
fname == "-"
# always lint explicitly passed (even if not matching filter)
or arg == fname
# otherwise, check the file against filtered patterns
or utils.fnmatch(fname, filename_patterns)
)
return (
filename
for path in paths
for filename in _filenames_from(path, predicate=is_excluded)
if is_included(path, filename)
if (
# always lint `-`
filename == "-"
# always lint explicitly passed (even if not matching filter)
or path == filename
# otherwise, check the file against filtered patterns
or utils.fnmatch(filename, filename_patterns)
)
)

View file

@ -13,7 +13,6 @@ from flake8 import checker
from flake8 import defaults
from flake8 import exceptions
from flake8 import style_guide
from flake8 import utils
from flake8.formatting.base import BaseFormatter
from flake8.main import debug
from flake8.main import options
@ -66,9 +65,6 @@ class Application:
#: with a non-zero status code
self.catastrophic_failure = False
#: The parsed diff information
self.parsed_diff: dict[str, set[int]] = {}
def parse_preliminary_options(
self, argv: Sequence[str]
) -> tuple[argparse.Namespace, list[str]]:
@ -158,13 +154,6 @@ class Application:
print(json.dumps(info, indent=2, sort_keys=True))
raise SystemExit(0)
if self.options.diff:
LOG.warning(
"the --diff option is deprecated and will be removed in a "
"future version."
)
self.parsed_diff = utils.parse_unified_diff()
for loaded in self.plugins.all_plugins():
parse_options = getattr(loaded.obj, "parse_options", None)
if parse_options is None:
@ -194,9 +183,6 @@ class Application:
self.options, self.formatter
)
if self.options.diff:
self.guide.add_diff_ranges(self.parsed_diff)
def make_file_checker_manager(self) -> None:
"""Initialize our FileChecker Manager."""
assert self.guide is not None
@ -213,16 +199,9 @@ class Application:
:class:`~flake8.checker.Manger` instance run the checks it is
managing.
"""
assert self.options is not None
assert self.file_checker_manager is not None
if self.options.diff:
files: list[str] | None = sorted(self.parsed_diff)
if not files:
return
else:
files = None
self.file_checker_manager.start(files)
self.file_checker_manager.start()
try:
self.file_checker_manager.run()
except exceptions.PluginExecutionFailed as plugin_failed:

View file

@ -114,7 +114,6 @@ def register_default_options(option_manager: OptionManager) -> None:
- ``-q``/``--quiet``
- ``--color``
- ``--count``
- ``--diff``
- ``--exclude``
- ``--extend-exclude``
- ``--filename``
@ -163,13 +162,6 @@ def register_default_options(option_manager: OptionManager) -> None:
"all other output.",
)
add_option(
"--diff",
action="store_true",
help="(DEPRECATED) Report changes only within line number ranges in "
"the unified diff provided on standard in by the user.",
)
add_option(
"--exclude",
metavar="patterns",

View file

@ -298,18 +298,6 @@ class StyleGuideManager:
code, filename, line_number, column_number, text, physical_line
)
def add_diff_ranges(self, diffinfo: dict[str, set[int]]) -> None:
"""Update the StyleGuides to filter out information not in the diff.
This provides information to the underlying StyleGuides so that only
the errors in the line number ranges are reported.
:param diffinfo:
Dictionary mapping filenames to sets of line number ranges.
"""
for guide in self.style_guides:
guide.add_diff_ranges(diffinfo)
class StyleGuide:
"""Manage a Flake8 user's style guide."""
@ -333,7 +321,6 @@ class StyleGuide:
self.filename = filename
if self.filename:
self.filename = utils.normalize_path(self.filename)
self._parsed_diff: dict[str, set[int]] = {}
def __repr__(self) -> str:
"""Make it easier to debug which StyleGuide we're using."""
@ -440,20 +427,8 @@ class StyleGuide:
self.should_report_error(error.code) is Decision.Selected
)
is_not_inline_ignored = error.is_inline_ignored(disable_noqa) is False
is_included_in_diff = error.is_in(self._parsed_diff)
if error_is_selected and is_not_inline_ignored and is_included_in_diff:
if error_is_selected and is_not_inline_ignored:
self.formatter.handle(error)
self.stats.record(error)
return 1
return 0
def add_diff_ranges(self, diffinfo: dict[str, set[int]]) -> None:
"""Update the StyleGuide to filter out information not in the diff.
This provides information to the StyleGuide so that only the errors
in the line number ranges are reported.
:param diffinfo:
Dictionary mapping filenames to sets of line number ranges.
"""
self._parsed_diff = diffinfo

View file

@ -1,7 +1,6 @@
"""Utility methods for flake8."""
from __future__ import annotations
import collections
import fnmatch as _fnmatch
import functools
import io
@ -18,7 +17,6 @@ from typing import Sequence
from flake8 import exceptions
DIFF_HUNK_REGEXP = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@.*$")
COMMA_SEPARATED_LIST_RE = re.compile(r"[,\s]")
LOCAL_PLUGIN_LIST_RE = re.compile(r"[,\t\n\r\f\v]")
NORMALIZE_PACKAGE_NAME_RE = re.compile(r"[-_.]+")
@ -202,71 +200,6 @@ def stdin_get_lines() -> list[str]:
return list(io.StringIO(stdin_get_value()))
def parse_unified_diff(diff: str | None = None) -> dict[str, set[int]]:
"""Parse the unified diff passed on stdin.
:returns:
dictionary mapping file names to sets of line numbers
"""
# Allow us to not have to patch out stdin_get_value
if diff is None:
diff = stdin_get_value()
number_of_rows = None
current_path = None
parsed_paths: dict[str, set[int]] = collections.defaultdict(set)
for line in diff.splitlines():
if number_of_rows:
if not line or line[0] != "-":
number_of_rows -= 1
# We're in the part of the diff that has lines starting with +, -,
# and ' ' to show context and the changes made. We skip these
# because the information we care about is the filename and the
# range within it.
# When number_of_rows reaches 0, we will once again start
# searching for filenames and ranges.
continue
# NOTE(sigmavirus24): Diffs that we support look roughly like:
# diff a/file.py b/file.py
# ...
# --- a/file.py
# +++ b/file.py
# Below we're looking for that last line. Every diff tool that
# gives us this output may have additional information after
# ``b/file.py`` which it will separate with a \t, e.g.,
# +++ b/file.py\t100644
# Which is an example that has the new file permissions/mode.
# In this case we only care about the file name.
if line[:3] == "+++":
current_path = line[4:].split("\t", 1)[0]
# NOTE(sigmavirus24): This check is for diff output from git.
if current_path[:2] == "b/":
current_path = current_path[2:]
# We don't need to do anything else. We have set up our local
# ``current_path`` variable. We can skip the rest of this loop.
# The next line we will see will give us the hung information
# which is in the next section of logic.
continue
hunk_match = DIFF_HUNK_REGEXP.match(line)
# NOTE(sigmavirus24): pep8/pycodestyle check for:
# line[:3] == '@@ '
# But the DIFF_HUNK_REGEXP enforces that the line start with that
# So we can more simply check for a match instead of slicing and
# comparing.
if hunk_match:
(row, number_of_rows) = (
1 if not group else int(group) for group in hunk_match.groups()
)
assert current_path is not None
parsed_paths[current_path].update(range(row, row + number_of_rows))
# We have now parsed our diff into a dictionary that looks like:
# {'file.py': set(range(10, 16), range(18, 20)), ...}
return parsed_paths
def is_using_stdin(paths: list[str]) -> bool:
"""Determine if we're going to read from stdin.

View file

@ -67,36 +67,3 @@ class Violation(NamedTuple):
"%r is not ignored inline with ``# noqa: %s``", self, codes_str
)
return False
def is_in(self, diff: dict[str, set[int]]) -> bool:
"""Determine if the violation is included in a diff's line ranges.
This function relies on the parsed data added via
:meth:`~StyleGuide.add_diff_ranges`. If that has not been called and
we are not evaluating files in a diff, then this will always return
True. If there are diff ranges, then this will return True if the
line number in the error falls inside one of the ranges for the file
(and assuming the file is part of the diff data). If there are diff
ranges, this will return False if the file is not part of the diff
data or the line number of the error is not in any of the ranges of
the diff.
:returns:
True if there is no diff or if the error is in the diff's line
number ranges. False if the error's line number falls outside
the diff's line number ranges.
"""
if not diff:
return True
# NOTE(sigmavirus24): The parsed diff will be a defaultdict with
# a set as the default value (if we have received it from
# flake8.utils.parse_unified_diff). In that case ranges below
# could be an empty set (which is False-y) or if someone else
# is using this API, it could be None. If we could guarantee one
# or the other, we would check for it more explicitly.
line_numbers = diff.get(self.filename)
if not line_numbers:
return False
return self.line_number in line_numbers

View file

@ -1,130 +0,0 @@
diff --git a/flake8/utils.py b/flake8/utils.py
index f6ce384..7cd12b0 100644
--- a/flake8/utils.py
+++ b/flake8/utils.py
@@ -75,8 +75,8 @@ def stdin_get_value():
return cached_value.getvalue()
-def parse_unified_diff():
- # type: () -> List[str]
+def parse_unified_diff(diff=None):
+ # type: (str) -> List[str]
"""Parse the unified diff passed on stdin.
:returns:
@@ -84,7 +84,10 @@ def parse_unified_diff():
:rtype:
dict
"""
- diff = stdin_get_value()
+ # Allow us to not have to patch out stdin_get_value
+ if diff is None:
+ diff = stdin_get_value()
+
number_of_rows = None
current_path = None
parsed_paths = collections.defaultdict(set)
diff --git a/tests/fixtures/diffs/single_file_diff b/tests/fixtures/diffs/single_file_diff
new file mode 100644
index 0000000..77ca534
--- /dev/null
+++ b/tests/fixtures/diffs/single_file_diff
@@ -0,0 +1,27 @@
+diff --git a/flake8/utils.py b/flake8/utils.py
+index f6ce384..7cd12b0 100644
+--- a/flake8/utils.py
++++ b/flake8/utils.py
+@@ -75,8 +75,8 @@ def stdin_get_value():
+ return cached_value.getvalue()
+
+
+-def parse_unified_diff():
+- # type: () -> List[str]
++def parse_unified_diff(diff=None):
++ # type: (str) -> List[str]
+ """Parse the unified diff passed on stdin.
+
+ :returns:
+@@ -84,7 +84,10 @@ def parse_unified_diff():
+ :rtype:
+ dict
+ """
+- diff = stdin_get_value()
++ # Allow us to not have to patch out stdin_get_value
++ if diff is None:
++ diff = stdin_get_value()
++
+ number_of_rows = None
+ current_path = None
+ parsed_paths = collections.defaultdict(set)
diff --git a/tests/fixtures/diffs/two_file_diff b/tests/fixtures/diffs/two_file_diff
new file mode 100644
index 0000000..5bd35cd
--- /dev/null
+++ b/tests/fixtures/diffs/two_file_diff
@@ -0,0 +1,45 @@
+diff --git a/flake8/utils.py b/flake8/utils.py
+index f6ce384..7cd12b0 100644
+--- a/flake8/utils.py
++++ b/flake8/utils.py
+@@ -75,8 +75,8 @@ def stdin_get_value():
+ return cached_value.getvalue()
+
+
+-def parse_unified_diff():
+- # type: () -> List[str]
++def parse_unified_diff(diff=None):
++ # type: (str) -> List[str]
+ """Parse the unified diff passed on stdin.
+
+ :returns:
+@@ -84,7 +84,10 @@ def parse_unified_diff():
+ :rtype:
+ dict
+ """
+- diff = stdin_get_value()
++ # Allow us to not have to patch out stdin_get_value
++ if diff is None:
++ diff = stdin_get_value()
++
+ number_of_rows = None
+ current_path = None
+ parsed_paths = collections.defaultdict(set)
+diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
+index d69d939..21482ce 100644
+--- a/tests/unit/test_utils.py
++++ b/tests/unit/test_utils.py
+@@ -115,3 +115,13 @@ def test_parameters_for_function_plugin():
+ plugin = plugin_manager.Plugin('plugin-name', object())
+ plugin._plugin = fake_plugin
+ assert utils.parameters_for(plugin) == ['physical_line', 'self', 'tree']
++
++
++def read_diff_file(filename):
++ """Read the diff file in its entirety."""
++ with open(filename, 'r') as fd:
++ content = fd.read()
++ return content
++
++
++SINGLE_FILE_DIFF = read_diff_file('tests/fixtures/diffs/single_file_diff')
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index d69d939..1461369 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -115,3 +115,14 @@ def test_parameters_for_function_plugin():
plugin = plugin_manager.Plugin('plugin-name', object())
plugin._plugin = fake_plugin
assert utils.parameters_for(plugin) == ['physical_line', 'self', 'tree']
+
+
+def read_diff_file(filename):
+ """Read the diff file in its entirety."""
+ with open(filename, 'r') as fd:
+ content = fd.read()
+ return content
+
+
+SINGLE_FILE_DIFF = read_diff_file('tests/fixtures/diffs/single_file_diff')
+TWO_FILE_DIFF = read_diff_file('tests/fixtures/diffs/two_file_diff')

View file

@ -1,27 +0,0 @@
diff --git a/flake8/utils.py b/flake8/utils.py
index f6ce384..7cd12b0 100644
--- a/flake8/utils.py
+++ b/flake8/utils.py
@@ -75,8 +75,8 @@ def stdin_get_value():
return cached_value.getvalue()
-def parse_unified_diff():
- # type: () -> List[str]
+def parse_unified_diff(diff=None):
+ # type: (str) -> List[str]
"""Parse the unified diff passed on stdin.
:returns:
@@ -84,7 +84,10 @@ def parse_unified_diff():
:rtype:
dict
"""
- diff = stdin_get_value()
+ # Allow us to not have to patch out stdin_get_value
+ if diff is None:
+ diff = stdin_get_value()
+
number_of_rows = None
current_path = None
parsed_paths = collections.defaultdict(set)

View file

@ -1,45 +0,0 @@
diff --git a/flake8/utils.py b/flake8/utils.py
index f6ce384..7cd12b0 100644
--- a/flake8/utils.py
+++ b/flake8/utils.py
@@ -75,8 +75,8 @@ def stdin_get_value():
return cached_value.getvalue()
-def parse_unified_diff():
- # type: () -> List[str]
+def parse_unified_diff(diff=None):
+ # type: (str) -> List[str]
"""Parse the unified diff passed on stdin.
:returns:
@@ -84,7 +84,10 @@ def parse_unified_diff():
:rtype:
dict
"""
- diff = stdin_get_value()
+ # Allow us to not have to patch out stdin_get_value
+ if diff is None:
+ diff = stdin_get_value()
+
number_of_rows = None
current_path = None
parsed_paths = collections.defaultdict(set)
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index d69d939..21482ce 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -115,3 +115,13 @@ def test_parameters_for_function_plugin():
plugin = plugin_manager.Plugin('plugin-name', object())
plugin._plugin = fake_plugin
assert utils.parameters_for(plugin) == ['physical_line', 'self', 'tree']
+
+
+def read_diff_file(filename):
+ """Read the diff file in its entirety."""
+ with open(filename, 'r') as fd:
+ content = fd.read()
+ return content
+
+
+SINGLE_FILE_DIFF = read_diff_file('tests/fixtures/diffs/single_file_diff')

View file

@ -13,42 +13,6 @@ from flake8.main import cli
from flake8.options import config
def test_diff_option(tmpdir, capsys):
"""Ensure that `flake8 --diff` works."""
t_py_contents = """\
import os
import sys # unused but not part of diff
print('(to avoid trailing whitespace in test)')
print('(to avoid trailing whitespace in test)')
print(os.path.join('foo', 'bar'))
y # part of the diff and an error
"""
diff = """\
diff --git a/t.py b/t.py
index d64ac39..7d943de 100644
--- a/t.py
+++ b/t.py
@@ -4,3 +4,5 @@ import sys # unused but not part of diff
print('(to avoid trailing whitespace in test)')
print('(to avoid trailing whitespace in test)')
print(os.path.join('foo', 'bar'))
+
+y # part of the diff and an error
"""
with mock.patch.object(utils, "stdin_get_value", return_value=diff):
with tmpdir.as_cwd():
tmpdir.join("t.py").write(t_py_contents)
assert cli.main(["--diff"]) == 1
out, err = capsys.readouterr()
assert out == "t.py:8:1: F821 undefined name 'y'\n"
assert err == ""
def test_form_feed_line_split(tmpdir, capsys):
"""Test that form feed is treated the same for stdin."""
src = "x=1\n\f\ny=1\n"

View file

@ -14,12 +14,7 @@ from flake8.plugins import finder
def style_guide_mock():
"""Create a mock StyleGuide object."""
return mock.MagicMock(
**{
"options.diff": False,
"options.jobs": JobsArgument("4"),
}
)
return mock.MagicMock(**{"options.jobs": JobsArgument("4")})
def _parallel_checker_manager():

View file

@ -125,7 +125,6 @@ def _expand_paths(
stdin_display_name="stdin",
filename_patterns=("*.py",),
exclude=(),
is_running_from_diff=False,
):
return set(
expand_paths(
@ -133,7 +132,6 @@ def _expand_paths(
stdin_display_name=stdin_display_name,
filename_patterns=filename_patterns,
exclude=exclude,
is_running_from_diff=is_running_from_diff,
)
)
@ -166,11 +164,3 @@ def test_alternate_stdin_name_is_filtered():
def test_filename_included_even_if_not_matching_include(tmp_path):
some_file = str(tmp_path.joinpath("some/file"))
assert _expand_paths(paths=(some_file,)) == {some_file}
def test_diff_filenames_filtered_by_patterns(tmp_path):
f1 = str(tmp_path.joinpath("f1"))
f2 = str(tmp_path.joinpath("f2.py"))
ret = _expand_paths(paths=(f1, f2), is_running_from_diff=True)
assert ret == {f2}

View file

@ -183,44 +183,6 @@ def test_fnmatch(filename, patterns, expected):
assert utils.fnmatch(filename, patterns) is expected
def read_diff_file(filename):
"""Read the diff file in its entirety."""
with open(filename) as fd:
content = fd.read()
return content
SINGLE_FILE_DIFF = read_diff_file("tests/fixtures/diffs/single_file_diff")
SINGLE_FILE_INFO = {
"flake8/utils.py": set(range(75, 83)).union(set(range(84, 94))),
}
TWO_FILE_DIFF = read_diff_file("tests/fixtures/diffs/two_file_diff")
TWO_FILE_INFO = {
"flake8/utils.py": set(range(75, 83)).union(set(range(84, 94))),
"tests/unit/test_utils.py": set(range(115, 128)),
}
MULTI_FILE_DIFF = read_diff_file("tests/fixtures/diffs/multi_file_diff")
MULTI_FILE_INFO = {
"flake8/utils.py": set(range(75, 83)).union(set(range(84, 94))),
"tests/unit/test_utils.py": set(range(115, 129)),
"tests/fixtures/diffs/single_file_diff": set(range(1, 28)),
"tests/fixtures/diffs/two_file_diff": set(range(1, 46)),
}
@pytest.mark.parametrize(
"diff, parsed_diff",
[
(SINGLE_FILE_DIFF, SINGLE_FILE_INFO),
(TWO_FILE_DIFF, TWO_FILE_INFO),
(MULTI_FILE_DIFF, MULTI_FILE_INFO),
],
)
def test_parse_unified_diff(diff, parsed_diff):
"""Verify that what we parse from a diff matches expectations."""
assert utils.parse_unified_diff(diff) == parsed_diff
def test_stdin_get_value_crlf():
"""Ensure that stdin is normalized from crlf to lf."""
stdin = io.TextIOWrapper(io.BytesIO(b"1\r\n2\r\n"), "UTF-8")

View file

@ -51,22 +51,3 @@ def test_disable_is_inline_ignored():
assert error.is_inline_ignored(True) is False
assert getline.called is False
@pytest.mark.parametrize(
"violation_file,violation_line,diff,expected",
[
("file.py", 10, {}, True),
("file.py", 1, {"file.py": range(1, 2)}, True),
("file.py", 10, {"file.py": range(1, 2)}, False),
("file.py", 1, {"other.py": range(1, 2)}, False),
("file.py", 10, {"other.py": range(1, 2)}, False),
],
)
def test_violation_is_in_diff(violation_file, violation_line, diff, expected):
"""Verify that we find violations within a diff."""
violation = Violation(
"E001", violation_file, violation_line, 1, "warning", "line"
)
assert violation.is_in(diff) is expected