From 9ee3e141b0eb7c791867600fa7cb461ec6b157e3 Mon Sep 17 00:00:00 2001 From: dosisod <39638017+dosisod@users.noreply.github.com> Date: Mon, 3 Oct 2022 21:26:26 -0700 Subject: [PATCH] Add ability to enable/disable checks: There is now a new "enabled" flag which can be used to skip the loading of a check unless the user explictly opts into the check. By default, all checks are enabled unless the author disables them. I only added --enable, since adding --disable will require some additional logic for parsing. Either way, --ignore does almost the same thing, but --disable would prevent the check from even being loaded, whereas --ignore only suppresses an error. --- refurb/error.py | 1 + refurb/loader.py | 16 +++++++++++---- refurb/main.py | 2 +- refurb/settings.py | 19 ++++++++++++++++++ test/custom_checks/disabled_check.py | 17 ++++++++++++++++ test/test_arg_parsing.py | 30 +++++++++++++++++++++++++--- test/test_checks.py | 26 ++++++++++++++++++++++++ 7 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 test/custom_checks/disabled_check.py diff --git a/refurb/error.py b/refurb/error.py index 72419b9..d75e97a 100644 --- a/refurb/error.py +++ b/refurb/error.py @@ -19,6 +19,7 @@ def __str__(self) -> str: @dataclass class Error: + enabled: ClassVar[bool] = True prefix: ClassVar[str] = "FURB" code: ClassVar[int] line: int diff --git a/refurb/loader.py b/refurb/loader.py index 09b3969..88d3caa 100644 --- a/refurb/loader.py +++ b/refurb/loader.py @@ -13,6 +13,7 @@ from . import checks as checks_module from .error import Error, ErrorCode +from .settings import Settings Check = Callable[[Node, list[Error]], None] @@ -40,15 +41,22 @@ def get_error_class(module: ModuleType) -> type[Error] | None: return None -def load_checks( - ignore: set[ErrorCode], paths: list[str] -) -> defaultdict[Type[Node], list[Check]]: +def load_checks(settings: Settings) -> defaultdict[Type[Node], list[Check]]: found: defaultdict[Type[Node], list[Check]] = defaultdict(list) + ignore = settings.ignore or set() + paths = settings.load or [] + enabled = settings.enable or set() for module in get_modules(paths): error = get_error_class(module) + if not error: + continue + + error_code = ErrorCode.from_error(error) + + is_enabled = error.enabled or error_code in enabled - if not error or ErrorCode.from_error(error) in ignore: + if not is_enabled or error_code in ignore: continue if func := getattr(module, "check", None): diff --git a/refurb/main.py b/refurb/main.py index 15f0021..5ee438a 100644 --- a/refurb/main.py +++ b/refurb/main.py @@ -103,7 +103,7 @@ def run_refurb(settings: Settings) -> Sequence[Error | str]: if settings.debug: errors.append(str(tree)) - checks = load_checks(settings.ignore or set(), settings.load or []) + checks = load_checks(settings) visitor = RefurbVisitor(checks) tree.accept(visitor) diff --git a/refurb/settings.py b/refurb/settings.py index 608efc7..a1363a3 100644 --- a/refurb/settings.py +++ b/refurb/settings.py @@ -17,6 +17,7 @@ class Settings: explain: ErrorCode | None = None ignore: set[ErrorCode] | None = None load: list[str] | None = None + enable: set[ErrorCode] | None = None debug: bool = False generate: bool = False help: bool = False @@ -45,8 +46,13 @@ def parse_config_file(contents: str) -> Settings: parse_error_id(str(x)) for x in settings.get("ignore", []) ) + enable = set( + parse_error_id(str(x)) for x in settings.get("enable", []) + ) + return Settings( ignore=ignore or None, + enable=enable or None, load=settings.get("load"), quiet=settings.get("quiet", False), ) @@ -67,6 +73,7 @@ def parse_command_line_args(args: list[str]) -> Settings: iargs = iter(args) files: list[str] = [] ignore: set[ErrorCode] = set() + enable: set[ErrorCode] = set() load: list[str] = [] explain: ErrorCode | None = None debug = False @@ -95,6 +102,14 @@ def parse_command_line_args(args: list[str]) -> Settings: ignore.add(parse_error_id(value)) + elif arg == "--enable": + value = next(iargs, None) + + if value is None: + raise ValueError(f'refurb: missing argument after "{arg}"') + + enable.add(parse_error_id(value)) + elif arg == "--load": value = next(iargs, None) @@ -112,6 +127,7 @@ def parse_command_line_args(args: list[str]) -> Settings: return Settings( files=files or None, ignore=ignore or None, + enable=enable or None, load=load or None, debug=debug, explain=explain, @@ -123,6 +139,9 @@ def merge_settings(command_line: Settings, config_file: Settings) -> Settings: if not command_line.ignore: command_line.ignore = config_file.ignore + if not command_line.enable: + command_line.enable = config_file.enable + if not command_line.load: command_line.load = config_file.load diff --git a/test/custom_checks/disabled_check.py b/test/custom_checks/disabled_check.py new file mode 100644 index 0000000..af8fa5c --- /dev/null +++ b/test/custom_checks/disabled_check.py @@ -0,0 +1,17 @@ +from dataclasses import dataclass + +from mypy.nodes import MypyFile + +from refurb.error import Error + + +@dataclass +class ErrorInfo(Error): + enabled = False + prefix = "XYZ" + code = 999 + msg: str = "This message is disabled by default" + + +def check(node: MypyFile, errors: list[Error]) -> None: + errors.append(ErrorInfo(node.line, node.column)) diff --git a/test/test_arg_parsing.py b/test/test_arg_parsing.py index 9a539c4..5a8492d 100644 --- a/test/test_arg_parsing.py +++ b/test/test_arg_parsing.py @@ -64,6 +64,20 @@ def test_parse_ignore_check_missing_arg() -> None: parse_args(["--ignore"]) +def test_parse_enable() -> None: + got = parse_args(["--enable", "FURB123", "--enable", "321"]) + expected = Settings(enable=set((ErrorCode(123), ErrorCode(321)))) + + assert got == expected + + +def test_parse_enable_check_missing_arg() -> None: + with pytest.raises( + ValueError, match='refurb: missing argument after "--enable"' + ): + parse_args(["--enable"]) + + def test_debug_parsing() -> None: assert parse_args(["--debug", "file"]) == Settings( files=["file"], debug=True @@ -98,12 +112,15 @@ def test_parse_config_file() -> None: [tool.refurb] load = ["some", "folders"] ignore = [100, "FURB101"] +enable = ["FURB111", "FURB222"] """ config = parse_config_file(contents) assert config == Settings( - load=["some", "folders"], ignore=set((ErrorCode(100), ErrorCode(101))) + load=["some", "folders"], + ignore=set((ErrorCode(100), ErrorCode(101))), + enable=set((ErrorCode(111), ErrorCode(222))), ) @@ -131,14 +148,21 @@ def test_command_line_args_override_config_file() -> None: [tool.refurb] load = ["some", "folders"] ignore = [100, "FURB101"] +enable = ["FURB111", "FURB222"] """ - command_line_args = parse_args(["--load", "x", "--ignore", "123"]) + command_line_args = parse_args( + ["--load", "x", "--ignore", "123", "--enable", "FURB200"] + ) config_file = parse_config_file(contents) config = merge_settings(command_line_args, config_file) - assert config == Settings(load=["x"], ignore=set((ErrorCode(123),))) + assert config == Settings( + load=["x"], + ignore=set((ErrorCode(123),)), + enable=set((ErrorCode(200),)), + ) def test_config_missing_ignore_option_is_allowed() -> None: diff --git a/test/test_checks.py b/test/test_checks.py index 97546f1..0f95bd5 100644 --- a/test/test_checks.py +++ b/test/test_checks.py @@ -69,3 +69,29 @@ def test_system_exit_is_caught() -> None: assert errors == [ "refurb: There are no .py[i] files in directory 'test/e2e/empty_package'" ] + + +DISABLED_CHECK = "test.custom_checks.disabled_check" + + +def test_disabled_check_is_not_ran_by_default() -> None: + errors = run_refurb( + Settings(files=["test/e2e/dummy.py"], load=[DISABLED_CHECK]) + ) + + assert not errors + + +def test_disabled_check_ran_if_explicitly_enabled() -> None: + errors = run_refurb( + Settings( + files=["test/e2e/dummy.py"], + load=[DISABLED_CHECK], + enable=set((ErrorCode(prefix="XYZ", id=999),)), + ) + ) + + expected = "test/e2e/dummy.py:1:1 [XYZ999]: This message is disabled by default" # noqa: E501 + + assert len(errors) == 1 + assert str(errors[0]) == expected