From 98938ef16b166789a7d58759dee4f7bae5037a1e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 19 Dec 2022 13:17:51 +0100 Subject: [PATCH] add pre-commit config, which replaces and extends 'tox -e check' adding several flake8 plugins pyright is not run with the pre-commit ci, since it requires an internet connection and the free tier for FOSS projects does not allow it. So it's called by tox in a separate GitHub action also fixes several minor style violations reported by those plugins, and adding a few comments --- .github/workflows/ci.yml | 11 +++--- .pre-commit-config.yaml | 67 ++++++++++++++++++++++++++++++++++ flake8_trio.py | 31 ++++++++-------- tests/conftest.py | 2 +- tests/test_decorator.py | 12 +++---- tests/test_flake8_trio.py | 75 +++++++++++++++++++++++++-------------- tox.ini | 38 +++++--------------- 7 files changed, 149 insertions(+), 87 deletions(-) create mode 100644 .pre-commit-config.yaml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0684aef..a67898a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,7 +7,7 @@ on: branches: [ main ] jobs: - check: + typing: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -18,11 +18,10 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip setuptools tox - python -m tox --notest --recreate -e check - - name: Run checks + python -m tox --notest --recreate -e typing + - name: Run type checks run: | - python -m tox -e check - git diff --exit-code + python -m tox -e typing test: runs-on: ubuntu-latest @@ -45,7 +44,7 @@ jobs: release: runs-on: ubuntu-latest - needs: [check, test] + needs: [typing, test] if: github.repository == 'Zac-HD/flake8-trio' && github.ref == 'refs/heads/main' steps: - uses: actions/checkout@v3 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..82b6bf1 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,67 @@ +ci: + skip: [pyright] + +repos: +- repo: https://github.com/Zac-HD/shed + rev: 0.10.8 + hooks: + - id: shed + args: ['--py39-plus'] + +- repo: https://github.com/RobertCraigie/pyright-python + rev: v1.1.286 + hooks: + - id: pyright + entry: env PYRIGHT_PYTHON_FORCE_VERSION=latest pyright + args: ['--pythonversion=3.11', '--warnings'] + additional_dependencies: + # Required for pyright strict mode + - hypothesis + - hypothesmith + - pytest + - flake8 + +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.4.0 + hooks: + - id: trailing-whitespace + args: ['--markdown-linebreak-ext=md,markdown'] + - id: end-of-file-fixer + - id: fix-encoding-pragma + args: [--remove] + - id: check-yaml + - id: debug-statements + language_version: python3 + +- repo: https://github.com/PyCQA/flake8 + rev: 6.0.0 + hooks: + - id: flake8 + args: ["--exclude", ".*,tests/trio*.py"] + language_version: python3 + additional_dependencies: + - flake8-builtins + - flake8-bugbear + - flake8-comprehensions + - flake8-2020 + - flake8-bandit + - flake8-builtins + - flake8-bugbear + - flake8-comprehensions + - flake8-datetimez + #- flake8-docstrings + - flake8-mutable + - flake8-noqa + - flake8-pie + - flake8-pytest-style + - flake8-return + - flake8-simplify + +- repo: https://github.com/PyCQA/flake8 + rev: 5.0.4 + hooks: + - id: flake8 + args: ["--exclude", ".*,tests/trio*.py", "--select=E800"] + language_version: python3 + additional_dependencies: + - flake8-eradicate diff --git a/flake8_trio.py b/flake8_trio.py index 1601840..8691bbe 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -49,7 +49,7 @@ def __eq__(self, other: Any) -> bool: # convenience function used in a lot of visitors def get_matching_call( - node: ast.AST, *names: str, base: str = "trio" + node: ast.AST, *names: str, base: int = "trio" ) -> tuple[ast.Call, str] | None: if ( isinstance(node, ast.Call) @@ -274,12 +274,11 @@ def walk(self, *body: ast.AST) -> Iterable[ast.AST]: # ignores module and only checks the unqualified name of the decorator # used in 101 and 107/108 def has_decorator(decorator_list: list[ast.expr], *names: str): - for dec in decorator_list: - if (isinstance(dec, ast.Name) and dec.id in names) or ( - isinstance(dec, ast.Attribute) and dec.attr in names - ): - return True - return False + return any( + (isinstance(dec, ast.Name) and dec.id in names) + or (isinstance(dec, ast.Attribute) and dec.attr in names) + for dec in decorator_list + ) # matches the fully qualified name against fnmatch pattern @@ -612,17 +611,18 @@ def visit_For(self, node: ast.For | ast.While): if not self.unraised: return + # the following block is duplicated in Visitor107_108 infinite_loop = False if isinstance(node, ast.While): try: infinite_loop = body_guaranteed_once = bool(ast.literal_eval(node.test)) - except Exception: + except Exception: # noqa: PIE786 body_guaranteed_once = False self.visit_nodes(node.test) else: - body_guaranteed_once = iter_guaranteed_once(node.iter) self.visit_nodes(node.target) self.visit_nodes(node.iter) + body_guaranteed_once = iter_guaranteed_once(node.iter) self.save_state(node, "unraised_break", "unraised_continue") self.unraised_break = False @@ -667,11 +667,10 @@ def iter_guaranteed_once(iterable: ast.expr) -> bool: else: return True return False + if isinstance(iterable, ast.Constant): - try: - return len(iterable.value) > 0 - except Exception: - return False + return hasattr(iterable.value, "__len__") and len(iterable.value) > 0 + if isinstance(iterable, ast.Dict): for key, val in zip(iterable.keys, iterable.values): # {**{...}, **{<...>}} is parsed as {None: {...}, None: {<...>}} @@ -688,7 +687,7 @@ def iter_guaranteed_once(iterable: ast.expr) -> bool: ): try: return len(range(*[ast.literal_eval(a) for a in iterable.args])) > 0 - except Exception: + except Exception: # noqa: PIE786 return False return False @@ -975,11 +974,13 @@ def visit_loop(self, node: ast.While | ast.For | ast.AsyncFor): if not self.async_function: return # visit condition + + # the following block is duplicated in Visitor103_104 infinite_loop = False if isinstance(node, ast.While): try: infinite_loop = body_guaranteed_once = bool(ast.literal_eval(node.test)) - except Exception: + except Exception: # noqa: PIE786 body_guaranteed_once = False self.visit_nodes(node.test) else: diff --git a/tests/conftest.py b/tests/conftest.py index beeddb7..2bbd571 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -30,6 +30,6 @@ def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item item.add_marker(skip_fuzz) -@pytest.fixture +@pytest.fixture() def enable_visitor_codes_regex(request: pytest.FixtureRequest): return request.config.getoption("--enable-visitor-codes-regex") diff --git a/tests/test_decorator.py b/tests/test_decorator.py index aeeab7f..ece721a 100644 --- a/tests/test_decorator.py +++ b/tests/test_decorator.py @@ -12,8 +12,7 @@ def dec_list(*decorators: str) -> ast.Module: for dec in decorators: source += f"@{dec}\n" source += "async def f():\n bar()" - tree = ast.parse(source) - return tree + return ast.parse(source) def wrap(decorators: tuple[str, ...], decs2: str) -> str | None: @@ -86,8 +85,7 @@ def test_pep614(): def test_command_line_1(capfd): Application().run(common_flags + ["--no-checkpoint-warning-decorators=app.route"]) - out, err = capfd.readouterr() - assert not out and not err + assert capfd.readouterr() == ("", "") expected_out = ( @@ -101,11 +99,9 @@ def test_command_line_1(capfd): def test_command_line_2(capfd): Application().run(common_flags + ["--no-checkpoint-warning-decorators=app"]) - out, err = capfd.readouterr() - assert out == expected_out and not err + assert capfd.readouterr() == (expected_out, "") def test_command_line_3(capfd): Application().run(common_flags) - out, err = capfd.readouterr() - assert out == expected_out and not err + assert capfd.readouterr() == (expected_out, "") diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index 91f7b8e..059e66c 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -6,7 +6,7 @@ import os import re import site -import subprocess +import subprocess # noqa: S404 import sys import tokenize import unittest @@ -54,7 +54,7 @@ def check_version(test: str): major, minor = version_str[0], version_str[1:] v_i = sys.version_info if (v_i.major, v_i.minor) < (int(major), int(minor)): - raise unittest.SkipTest("v_i, major, minor") + pytest.skip(f"python version {v_i} smaller than {major}, {minor}") ERROR_CODES = { @@ -64,7 +64,7 @@ def check_version(test: str): } -@pytest.mark.parametrize("test, path", test_files) +@pytest.mark.parametrize(("test", "path"), test_files) def test_eval(test: str, path: str): # version check check_version(test) @@ -104,7 +104,7 @@ def test_eval(test: str, path: str): # Append a bunch of empty strings so string formatting gives garbage # instead of throwing an exception try: - args = eval( + args = eval( # noqa: S307 f"[{err_args}]", { "lineno": lineno, @@ -167,8 +167,7 @@ def test_eval(test: str, path: str): class SyncTransformer(ast.NodeTransformer): def visit_Await(self, node: ast.Await): - newnode = self.generic_visit(node.value) - return newnode + return self.generic_visit(node.value) def replace_async(self, node: ast.AST, target: type[ast.AST]) -> ast.AST: node = self.generic_visit(node) @@ -186,7 +185,7 @@ def visit_AsyncFor(self, node: ast.AST): return self.replace_async(node, ast.For) -@pytest.mark.parametrize("test, path", test_files) +@pytest.mark.parametrize(("test", "path"), test_files) def test_noerror_on_sync_code(test: str, path: str): if any(e in test for e in error_codes_ignored_when_checking_transformed_sync_code): return @@ -233,7 +232,7 @@ def assert_expected_errors( assert_correct_args(errors, expected_) # full check - unittest.TestCase().assertEqual(errors, expected_) + assert errors == expected_ # test tuple conversion and iter types assert_tuple_and_types(errors, expected_) @@ -365,24 +364,50 @@ def info_tuple(error: Error): def test_107_permutations(): - # since each test is so fast, and there's so many permutations, manually doing - # the permutations in a single test is much faster than the permutations from using - # pytest parametrization - and does not clutter up the output massively. + """ + since each test is so fast, and there's so many permutations, manually doing + the permutations in a single test is much faster than the permutations from using + pytest parametrization - and does not clutter up the output massively. + + generates code that looks like this, where a block content of `None` means the + block is excluded: + + async def foo(): + try: + await foo() | ... + except ValueError: + await foo() | ... | raise | return | None + except SyntaxError: + await foo() | ... | raise | return | None + except: + await foo() | ... | raise | return | None + else: + await foo() | ... | return | None + finally: + await foo() | ... | return | None + """ plugin = Plugin(ast.AST()) initialize_options(plugin, args=["--enable-visitor-codes-regex=TRIO107"]) check = "await foo()" + + # loop over all the possible content of the different blocks for try_, exc1, exc2, bare_exc, else_, finally_ in itertools.product( - (check, "..."), - (check, "...", "raise", "return", None), - (check, "...", "raise", "return", None), - (check, "...", "raise", "return", None), - (check, "...", "return", None), - (check, "...", "return", None), + (check, "..."), # try_ + (check, "...", "raise", "return", None), # exc1 + (check, "...", "raise", "return", None), # exc2 + (check, "...", "raise", "return", None), # bare_exc + (check, "...", "return", None), # else_ + (check, "...", "return", None), # finally_ ): + # exclude duplicate tests where there's a second exception block but no first if exc1 is None and exc2 is not None: continue + # syntax error if there's no exception block but there's finally and/or else + if exc1 is exc2 is bare_exc is None and (finally_ is None or else_ is not None): + continue + function_str = f"async def foo():\n try:\n {try_}\n" for arg, val in { @@ -395,13 +420,7 @@ def test_107_permutations(): if val is not None: function_str += f" {arg}:\n {val}\n" - try: - tree = ast.parse(function_str) - except Exception: - assert exc1 is exc2 is bare_exc is None and ( - finally_ is None or else_ is not None - ) - return + tree = ast.parse(function_str) # not a type error per se, but it's pyright warning about assigning to a # protected class member - hence we silence it with a `type: ignore`. @@ -497,7 +516,9 @@ def test_200_from_config_flake8_internals( def test_200_from_config_subprocess(tmp_path: Path): err_msg = _test_trio200_from_config_common(tmp_path) - res = subprocess.run(["flake8"], cwd=tmp_path, capture_output=True) + res = subprocess.run( # noqa: S603,S607 + ["flake8"], cwd=tmp_path, capture_output=True + ) assert not res.stderr assert res.stdout == err_msg.encode("ascii") @@ -507,7 +528,7 @@ def consume(iterator: Iterable[Any]): deque(iterator, maxlen=0) -@pytest.mark.fuzz +@pytest.mark.fuzz() class TestFuzz(unittest.TestCase): @settings(max_examples=1_000, suppress_health_check=[HealthCheck.too_slow]) @given((from_grammar() | from_node()).map(ast.parse)) @@ -540,7 +561,7 @@ def _iter_python_files(): yield Path(dirname) / f -@pytest.mark.fuzz +@pytest.mark.fuzz() def test_does_not_crash_on_site_code(enable_visitor_codes_regex: str): for path in _iter_python_files(): try: diff --git a/tox.ini b/tox.ini index 91c6566..d4e7b4f 100644 --- a/tox.ini +++ b/tox.ini @@ -1,36 +1,7 @@ # The test environment and commands [tox] # default environments to run without `-e` -envlist = check, py{39,310,311}-{flake8_5,flake8_6} - -[testenv:check] -description = Format code and run linters (quick) -deps = - shed - flake8 - flake8-builtins - flake8-bugbear - flake8-comprehensions - pyright - # Required for pyright strict mode - hypothesis - hypothesmith - pytest - trio -setenv = - # Make sure pyright is always up to date - PYRIGHT_PYTHON_FORCE_VERSION = latest -skip_install = - # don't install the plugin, which would register it with flake8 - # and potentially stop the linter from functioning. - true -ignore_errors = - # true means "run all, fail if any failed" - true -commands = - shed --py39-plus - flake8 --exclude .*,tests/trio*.py - pyright --pythonversion 3.11 --warnings +envlist = py{39,310,311}-{flake8_5,flake8_6} # create a default testenv, whose behaviour will depend on the name it's called with. # for CI you can call with `-e flake8_5,flake8_6` and let the CI handle python version @@ -48,6 +19,12 @@ deps = commands = pytest {posargs} #{posargs:-n auto} +[testenv:typing] +description = Runs pyright in pre-commit from tox for CI, since it requires an internet connection +deps = + pre-commit +commands = + pre-commit run pyright --all-files # Settings for other tools [pytest] @@ -63,6 +40,7 @@ filterwarnings = [flake8] max-line-length = 90 +extend-ignore = S101 [coverage:report] exclude_lines =