From c5225db626bd4eda1d3852d54ecba5adaf927508 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Tue, 22 Mar 2022 18:10:32 -0700 Subject: [PATCH] simplify decision engine - not specified codes (cmdline / config) are now known as being implicit via None sentinel - removed redundant logic for (explicit, explicit) selection --- src/flake8/main/options.py | 56 +++--- src/flake8/options/aggregator.py | 20 +-- src/flake8/style_guide.py | 182 ++++++++------------ tests/integration/test_aggregator.py | 12 +- tests/unit/test_decision_engine.py | 247 +++++++-------------------- 5 files changed, 166 insertions(+), 351 deletions(-) diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py index 3479893..4c9dfb8 100644 --- a/src/flake8/main/options.py +++ b/src/flake8/main/options.py @@ -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( diff --git a/src/flake8/options/aggregator.py b/src/flake8/options/aggregator.py index 4b40883..580def6 100644 --- a/src/flake8/options/aggregator.py +++ b/src/flake8/options/aggregator.py @@ -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(): diff --git a/src/flake8/style_guide.py b/src/flake8/style_guide.py index 5258485..d9f6dbc 100644 --- a/src/flake8/style_guide.py +++ b/src/flake8/style_guide.py @@ -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 diff --git a/tests/integration/test_aggregator.py b/tests/integration/test_aggregator.py index de9e6fb..d35266f 100644 --- a/tests/integration/test_aggregator.py +++ b/tests/integration/test_aggregator.py @@ -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/*")] diff --git a/tests/unit/test_decision_engine.py b/tests/unit/test_decision_engine.py index 19d574a..1280385 100644 --- a/tests/unit/test_decision_engine.py +++ b/tests/unit/test_decision_engine.py @@ -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