simplify decision engine

- not specified codes (cmdline / config) are now known as being implicit via
  None sentinel
- removed redundant logic for (explicit, explicit) selection
This commit is contained in:
Anthony Sottile 2022-03-22 18:10:32 -07:00
parent dba40df8d1
commit c5225db626
5 changed files with 166 additions and 351 deletions

View file

@ -24,8 +24,8 @@ def stage1_arg_parser() -> argparse.ArgumentParser:
"--verbose",
default=0,
action="count",
help="Print more information about what is happening in flake8."
" This option is repeatable and will increase verbosity each "
help="Print more information about what is happening in flake8. "
"This option is repeatable and will increase verbosity each "
"time it is repeated.",
)
@ -157,8 +157,8 @@ def register_default_options(option_manager: OptionManager) -> None:
"--count",
action="store_true",
parse_from_config=True,
help="Print total number of errors and warnings to standard output and"
" set the exit code to 1 if total is not empty.",
help="Print total number of errors to standard output and "
"set the exit code to 1 if total is not empty.",
)
add_option(
@ -175,8 +175,8 @@ def register_default_options(option_manager: OptionManager) -> None:
comma_separated_list=True,
parse_from_config=True,
normalize_paths=True,
help="Comma-separated list of files or directories to exclude."
" (Default: %(default)s)",
help="Comma-separated list of files or directories to exclude. "
"(Default: %(default)s)",
)
add_option(
@ -186,8 +186,8 @@ def register_default_options(option_manager: OptionManager) -> None:
parse_from_config=True,
comma_separated_list=True,
normalize_paths=True,
help="Comma-separated list of files or directories to add to the list"
" of excluded ones.",
help="Comma-separated list of files or directories to add to the list "
"of excluded ones.",
)
add_option(
@ -203,9 +203,9 @@ def register_default_options(option_manager: OptionManager) -> None:
add_option(
"--stdin-display-name",
default="stdin",
help="The name used when reporting errors from code passed via stdin."
" This is useful for editors piping the file contents to flake8."
" (Default: %(default)s)",
help="The name used when reporting errors from code passed via stdin. "
"This is useful for editors piping the file contents to flake8. "
"(Default: %(default)s)",
)
# TODO(sigmavirus24): Figure out --first/--repeat
@ -225,28 +225,29 @@ def register_default_options(option_manager: OptionManager) -> None:
"--hang-closing",
action="store_true",
parse_from_config=True,
help="Hang closing bracket instead of matching indentation of opening"
" bracket's line.",
help="Hang closing bracket instead of matching indentation of opening "
"bracket's line.",
)
add_option(
"--ignore",
metavar="errors",
default=",".join(defaults.IGNORE),
parse_from_config=True,
comma_separated_list=True,
help="Comma-separated list of errors and warnings to ignore (or skip)."
" For example, ``--ignore=E4,E51,W234``. (Default: %(default)s)",
help=(
f"Comma-separated list of error codes to ignore (or skip). "
f"For example, ``--ignore=E4,E51,W234``. "
f"(Default: {','.join(defaults.IGNORE)})"
),
)
add_option(
"--extend-ignore",
metavar="errors",
default="",
parse_from_config=True,
comma_separated_list=True,
help="Comma-separated list of errors and warnings to add to the list"
" of ignored ones. For example, ``--extend-ignore=E4,E51,W234``.",
help="Comma-separated list of error codes to add to the list of "
"ignored ones. For example, ``--extend-ignore=E4,E51,W234``.",
)
add_option(
@ -291,21 +292,22 @@ def register_default_options(option_manager: OptionManager) -> None:
add_option(
"--select",
metavar="errors",
default=",".join(defaults.SELECT),
parse_from_config=True,
comma_separated_list=True,
help="Comma-separated list of errors and warnings to enable."
" For example, ``--select=E4,E51,W234``. (Default: %(default)s)",
help=(
f"Comma-separated list of error codes to enable. "
f"For example, ``--select=E4,E51,W234``. "
f"(Default: {','.join(defaults.SELECT)})"
),
)
add_option(
"--extend-select",
metavar="errors",
default="",
parse_from_config=True,
comma_separated_list=True,
help=(
"Comma-separated list of errors and warnings to add to the list "
"Comma-separated list of error codes to add to the list "
"of selected ones. For example, ``--extend-select=E4,E51,W234``."
),
)
@ -339,7 +341,7 @@ def register_default_options(option_manager: OptionManager) -> None:
"--statistics",
action="store_true",
parse_from_config=True,
help="Count errors and warnings.",
help="Count errors.",
)
# Flake8 options
@ -358,8 +360,8 @@ def register_default_options(option_manager: OptionManager) -> None:
type=JobsArgument,
help="Number of subprocesses to use to run checks in parallel. "
'This is ignored on Windows. The default, "auto", will '
"auto-detect the number of processors available to use."
" (Default: %(default)s)",
"auto-detect the number of processors available to use. "
"(Default: %(default)s)",
)
add_option(

View file

@ -28,23 +28,9 @@ def aggregate_options(
# Get the parsed config
parsed_config = config.parse_config(manager, cfg, cfg_dir)
# Extend the default ignore value with the extended default ignore list,
# registered by plugins.
extended_default_ignore = manager.extended_default_ignore.copy()
# Let's store our extended default ignore for use by the decision engine
default_values.extended_default_ignore = (
manager.extended_default_ignore.copy()
)
LOG.debug("Extended default ignore list: %s", extended_default_ignore)
extended_default_ignore.extend(default_values.ignore)
default_values.ignore = extended_default_ignore
LOG.debug("Merged default ignore list: %s", default_values.ignore)
extended_default_select = manager.extended_default_select.copy()
LOG.debug(
"Extended default select list: %s", list(extended_default_select)
)
default_values.extended_default_select = extended_default_select
# store the plugin-set extended default ignore / select
default_values.extended_default_ignore = manager.extended_default_ignore
default_values.extended_default_select = manager.extended_default_select
# Merge values parsed from config onto the default values returned
for config_name, value in parsed_config.items():

View file

@ -47,6 +47,21 @@ class Decision(enum.Enum):
Selected = "selected error"
def _select_ignore(
*,
option: Optional[List[str]],
default: Tuple[str, ...],
extended_default: List[str],
extend: Optional[List[str]],
) -> Tuple[str, ...]:
# option was explicitly set, ignore the default and extended default
if option is not None:
ret = [*option, *(extend or [])]
else:
ret = [*default, *extended_default, *(extend or [])]
return tuple(sorted(ret, reverse=True))
class DecisionEngine:
"""A class for managing the decision process around violations.
@ -57,26 +72,27 @@ class DecisionEngine:
def __init__(self, options: argparse.Namespace) -> None:
"""Initialize the engine."""
self.cache: Dict[str, Decision] = {}
self.selected = tuple(options.select)
self.extended_selected = tuple(
sorted(options.extended_default_select, reverse=True)
self.using_default_select = (
options.select is None and options.extend_select is None
)
self.all_selected = tuple(
sorted(
itertools.chain(self.selected, options.extend_select),
reverse=True,
)
self.using_default_ignore = (
options.ignore is None and options.extend_ignore is None
)
self.ignored = tuple(
sorted(
itertools.chain(options.ignore, options.extend_ignore),
reverse=True,
)
self.selected = _select_ignore(
option=options.select,
default=defaults.SELECT,
extended_default=options.extended_default_select,
extend=options.extend_select,
)
self.ignored = _select_ignore(
option=options.ignore,
default=defaults.IGNORE,
extended_default=options.extended_default_ignore,
extend=options.extend_ignore,
)
self.using_default_ignore = set(self.ignored) == set(
defaults.IGNORE
).union(options.extended_default_ignore)
self.using_default_select = set(self.selected) == set(defaults.SELECT)
def was_selected(self, code: str) -> Union[Selected, Ignored]:
"""Determine if the code has been selected by the user.
@ -89,16 +105,13 @@ class DecisionEngine:
Ignored.Implicitly if the selected list is not empty but no match
was found.
"""
if code.startswith(self.all_selected):
return Selected.Explicitly
if not self.all_selected and code.startswith(self.extended_selected):
# If it was not explicitly selected, it may have been implicitly
# selected because the check comes from a plugin that is enabled by
# default
return Selected.Implicitly
return Ignored.Implicitly
if code.startswith(self.selected):
if self.using_default_select:
return Selected.Implicitly
else:
return Selected.Explicitly
else:
return Ignored.Implicitly
def was_ignored(self, code: str) -> Union[Selected, Ignored]:
"""Determine if the code has been ignored by the user.
@ -113,83 +126,54 @@ class DecisionEngine:
was found.
"""
if code.startswith(self.ignored):
return Ignored.Explicitly
return Selected.Implicitly
def more_specific_decision_for(self, code: str) -> Decision:
select = find_first_match(code, self.all_selected)
extra_select = find_first_match(code, self.extended_selected)
ignore = find_first_match(code, self.ignored)
if select and ignore:
# If the violation code appears in both the select and ignore
# lists (in some fashion) then if we're using the default ignore
# list and a custom select list we should select the code. An
# example usage looks like this:
# A user has a code that would generate an E126 violation which
# is in our default ignore list and they specify select=E.
# We should be reporting that violation. This logic changes,
# however, if they specify select and ignore such that both match.
# In that case we fall through to our find_more_specific call.
# If, however, the user hasn't specified a custom select, and
# we're using the defaults for both select and ignore then the
# more specific rule must win. In most cases, that will be to
# ignore the violation since our default select list is very
# high-level and our ignore list is highly specific.
if self.using_default_ignore and not self.using_default_select:
return Decision.Selected
return find_more_specific(select, ignore)
if extra_select and ignore:
# At this point, select is false-y. Now we need to check if the
# code is in our extended select list and our ignore list. This is
# a *rare* case as we see little usage of the extended select list
# that plugins can use, so I suspect this section may change to
# look a little like the block above in which we check if we're
# using our default ignore list.
return find_more_specific(extra_select, ignore)
if select or (extra_select and self.using_default_select):
# Here, ignore was false-y and the user has either selected
# explicitly the violation or the violation is covered by
# something in the extended select list and we're using the
# default select list. In either case, we want the violation to be
# selected.
return Decision.Selected
if select is None and (
extra_select is None or not self.using_default_ignore
):
return Decision.Ignored
if (select is None and not self.using_default_select) and (
ignore is None and self.using_default_ignore
):
return Decision.Ignored
return Decision.Selected
if self.using_default_ignore:
return Ignored.Implicitly
else:
return Ignored.Explicitly
else:
return Selected.Implicitly
def make_decision(self, code: str) -> Decision:
"""Decide if code should be ignored or selected."""
LOG.debug('Deciding if "%s" should be reported', code)
selected = self.was_selected(code)
ignored = self.was_ignored(code)
LOG.debug(
'The user configured "%s" to be "%s", "%s"',
"The user configured %r to be %r, %r",
code,
selected,
ignored,
)
if (
selected is Selected.Explicitly or selected is Selected.Implicitly
) and ignored is Selected.Implicitly:
decision = Decision.Selected
if isinstance(selected, Selected) and isinstance(ignored, Selected):
return Decision.Selected
elif isinstance(selected, Ignored) and isinstance(ignored, Ignored):
return Decision.Ignored
elif (
selected is Selected.Explicitly
and ignored is not Ignored.Explicitly
):
return Decision.Selected
elif (
selected is not Selected.Explicitly
and ignored is Ignored.Explicitly
):
return Decision.Ignored
elif selected is Ignored.Implicitly and ignored is Selected.Implicitly:
return Decision.Ignored
elif (
selected is Selected.Explicitly and ignored is Ignored.Explicitly
) or (
selected is Ignored.Implicitly and ignored is Selected.Implicitly
selected is Selected.Implicitly and ignored is Ignored.Implicitly
):
decision = self.more_specific_decision_for(code)
elif selected is Ignored.Implicitly or ignored is Ignored.Explicitly:
decision = Decision.Ignored # pylint: disable=R0204
return decision
# we only get here if it was in both lists: longest prefix wins
select = next(s for s in self.selected if code.startswith(s))
ignore = next(s for s in self.ignored if code.startswith(s))
if len(select) > len(ignore):
return Decision.Selected
else:
return Decision.Ignored
else:
raise AssertionError(f"unreachable {code} {selected} {ignored}")
def decision_for(self, code: str) -> Decision:
"""Return the decision for a specific code.
@ -362,7 +346,8 @@ class StyleGuide:
"""Create a copy of this style guide with different values."""
filename = filename or self.filename
options = copy.deepcopy(self.options)
options.ignore.extend(extend_ignore_with or [])
options.extend_ignore = options.extend_ignore or []
options.extend_ignore.extend(extend_ignore_with or [])
return StyleGuide(
options, self.formatter, self.stats, filename=filename
)
@ -471,20 +456,3 @@ class StyleGuide:
Dictionary mapping filenames to sets of line number ranges.
"""
self._parsed_diff = diffinfo
def find_more_specific(selected: str, ignored: str) -> Decision:
if selected.startswith(ignored) and selected != ignored:
return Decision.Selected
return Decision.Ignored
def find_first_match(
error_code: str, code_list: Tuple[str, ...]
) -> Optional[str]:
startswith = error_code.startswith
for code in code_list:
if startswith(code):
return code
else:
return None

View file

@ -76,15 +76,5 @@ def test_aggregate_options_when_isolated(optmanager, flake8_config):
options = aggregator.aggregate_options(optmanager, cfg, cfg_dir, arguments)
assert options.select == ["E11", "E34", "E402", "W", "F"]
assert sorted(options.ignore) == [
"E121",
"E123",
"E126",
"E226",
"E24",
"E704",
"E8",
"W503",
"W504",
]
assert options.ignore is None
assert options.exclude == [os.path.abspath("tests/*")]

View file

@ -3,18 +3,17 @@ import argparse
import pytest
from flake8 import defaults
from flake8 import style_guide
def create_options(**kwargs):
"""Create and return an instance of argparse.Namespace."""
kwargs.setdefault("select", [])
kwargs.setdefault("extended_default_ignore", [])
kwargs.setdefault("select", None)
kwargs.setdefault("ignore", None)
kwargs.setdefault("extend_select", None)
kwargs.setdefault("extend_ignore", None)
kwargs.setdefault("extended_default_select", [])
kwargs.setdefault("extend_select", [])
kwargs.setdefault("ignore", [])
kwargs.setdefault("extend_ignore", [])
kwargs.setdefault("extended_default_ignore", [])
kwargs.setdefault("disable_noqa", False)
return argparse.Namespace(**kwargs)
@ -93,7 +92,7 @@ def test_was_selected_implicitly_selects_errors():
error_code = "E121"
decider = style_guide.DecisionEngine(
create_options(
select=[],
select=None,
extended_default_select=["E"],
),
)
@ -121,13 +120,13 @@ def test_was_selected_excludes_errors(select_list, error_code):
@pytest.mark.parametrize(
"select_list,ignore_list,extend_ignore,error_code,expected",
[
(["E111", "E121"], [], [], "E111", style_guide.Decision.Selected),
(["E111", "E121"], [], [], "E112", style_guide.Decision.Ignored),
(["E111", "E121"], [], [], "E121", style_guide.Decision.Selected),
(["E111", "E121"], [], [], "E122", style_guide.Decision.Ignored),
(["E11", "E12"], [], [], "E132", style_guide.Decision.Ignored),
(["E2", "E12"], [], [], "E321", style_guide.Decision.Ignored),
(["E2", "E12"], [], [], "E410", style_guide.Decision.Ignored),
(["E111", "E121"], [], None, "E111", style_guide.Decision.Selected),
(["E111", "E121"], [], None, "E112", style_guide.Decision.Ignored),
(["E111", "E121"], [], None, "E121", style_guide.Decision.Selected),
(["E111", "E121"], [], None, "E122", style_guide.Decision.Ignored),
(["E11", "E12"], [], None, "E132", style_guide.Decision.Ignored),
(["E2", "E12"], [], None, "E321", style_guide.Decision.Ignored),
(["E2", "E12"], [], None, "E410", style_guide.Decision.Ignored),
(["E11", "E121"], ["E1"], [], "E112", style_guide.Decision.Selected),
(["E11", "E121"], [], ["E1"], "E112", style_guide.Decision.Selected),
(
@ -137,41 +136,41 @@ def test_was_selected_excludes_errors(select_list, error_code):
"E122",
style_guide.Decision.Ignored,
),
(["E11", "E12"], ["E13"], [], "E132", style_guide.Decision.Ignored),
(["E1", "E3"], ["E32"], [], "E321", style_guide.Decision.Ignored),
([], ["E2", "E12"], [], "E410", style_guide.Decision.Ignored),
(["E11", "E12"], ["E13"], None, "E132", style_guide.Decision.Ignored),
(["E1", "E3"], ["E32"], None, "E321", style_guide.Decision.Ignored),
([], ["E2", "E12"], None, "E410", style_guide.Decision.Ignored),
(
["E4"],
["E2", "E12", "E41"],
[],
None,
"E410",
style_guide.Decision.Ignored,
),
(
["E41"],
["E2", "E12", "E4"],
[],
None,
"E410",
style_guide.Decision.Selected,
),
(["E"], ["F"], [], "E410", style_guide.Decision.Selected),
(["F"], [], [], "E410", style_guide.Decision.Ignored),
(["E"], defaults.IGNORE, [], "E126", style_guide.Decision.Selected),
(["W"], defaults.IGNORE, [], "E126", style_guide.Decision.Ignored),
(["E"], defaults.IGNORE, [], "W391", style_guide.Decision.Ignored),
(["E", "W"], ["E13"], [], "E131", style_guide.Decision.Ignored),
(defaults.SELECT, ["E13"], [], "E131", style_guide.Decision.Ignored),
(["E"], ["F"], None, "E410", style_guide.Decision.Selected),
(["F"], [], None, "E410", style_guide.Decision.Ignored),
(["E"], None, None, "E126", style_guide.Decision.Selected),
(["W"], None, None, "E126", style_guide.Decision.Ignored),
(["E"], None, None, "W391", style_guide.Decision.Ignored),
(["E", "W"], ["E13"], None, "E131", style_guide.Decision.Ignored),
(None, ["E13"], None, "E131", style_guide.Decision.Ignored),
(
defaults.SELECT,
defaults.IGNORE,
None,
None,
["W391"],
"E126",
style_guide.Decision.Ignored,
),
(
defaults.SELECT,
defaults.IGNORE,
[],
None,
None,
None,
"W391",
style_guide.Decision.Selected,
),
@ -192,166 +191,36 @@ def test_decision_for(
assert decider.decision_for(error_code) is expected
@pytest.mark.parametrize(
(
"select",
"ignore",
"extended_default_ignore",
"extended_default_select",
"error_code",
"expected",
),
[
(
defaults.SELECT,
[],
[],
["I1"],
"I100",
style_guide.Decision.Selected,
),
(
defaults.SELECT,
[],
[],
["I1"],
"I201",
style_guide.Decision.Ignored,
),
(
defaults.SELECT,
["I2"],
[],
["I1"],
"I101",
style_guide.Decision.Selected,
),
(
defaults.SELECT,
["I2"],
[],
["I1"],
"I201",
style_guide.Decision.Ignored,
),
(
defaults.SELECT,
["I1"],
[],
["I10"],
"I101",
style_guide.Decision.Selected,
),
(
defaults.SELECT,
["I10"],
[],
["I1"],
"I101",
style_guide.Decision.Ignored,
),
(
defaults.SELECT,
["U401"],
[],
[],
"U401",
style_guide.Decision.Ignored,
),
(
["E", "W"],
["E13"],
[],
[],
"E131",
style_guide.Decision.Ignored,
),
(
["E", "W"],
["E13"],
[],
[],
"E126",
style_guide.Decision.Selected,
),
(["E2"], ["E21"], [], [], "E221", style_guide.Decision.Selected),
(["E2"], ["E21"], [], [], "E212", style_guide.Decision.Ignored),
(
["F", "W"],
["C90"],
[],
["I1"],
"C901",
style_guide.Decision.Ignored,
),
(
["E", "W"],
["C"],
[],
[],
"E131",
style_guide.Decision.Selected,
),
(
defaults.SELECT,
defaults.IGNORE,
[],
["G"],
"G101",
style_guide.Decision.Selected,
),
(
defaults.SELECT,
["G1"],
[],
["G"],
"G101",
style_guide.Decision.Ignored,
),
(
["E", "W"],
defaults.IGNORE,
[],
["I"],
"I101",
style_guide.Decision.Ignored,
),
(
["E", "W", "I101"],
defaults.IGNORE + ("I101",),
["I101"],
[],
"I101",
style_guide.Decision.Selected,
),
(
["E", "W"],
defaults.IGNORE + ("I101",),
["I101"],
[],
"I101",
style_guide.Decision.Ignored,
),
# TODO(sigmavirus24) Figure out how to exercise the final catch-all
# return statement
],
)
def test_more_specific_decision_for_logic(
select,
ignore,
extended_default_ignore,
extended_default_select,
error_code,
expected,
):
"""Verify the logic of DecisionEngine.more_specific_decision_for."""
def test_implicitly_selected_and_implicitly_ignored_defers_to_length():
decider = style_guide.DecisionEngine(
create_options(
select=select,
ignore=ignore,
extended_default_select=extended_default_select,
extended_default_ignore=extended_default_ignore,
# no options selected by user
select=None,
ignore=None,
extend_select=None,
extend_ignore=None,
# a plugin is installed and extends default ignore
extended_default_select=["P"],
extended_default_ignore=["P002"],
),
)
assert decider.more_specific_decision_for(error_code) is expected
assert decider.decision_for("P001") is style_guide.Decision.Selected
assert decider.decision_for("P002") is style_guide.Decision.Ignored
def test_user_can_extend_select_to_enable_plugin_default_ignored():
decider = style_guide.DecisionEngine(
create_options(
# user options --extend-select=P002
select=None,
ignore=None,
extend_select=["P002"],
extend_ignore=None,
# a plugin is installed and extends default ignore
extended_default_select=["P"],
extended_default_ignore=["P002"],
),
)
assert decider.decision_for("P002") is style_guide.Decision.Selected