diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index eb010753d..183612f36 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -1,5 +1,12 @@ # Unreleased +This major release removes `project:fix` and `project:format` +and replaces them with `format:fix` and `format:check`. + ## Refactoring * #606: Renamed nox session `project:fix` more aptly to `format:fix` and `project:format` to `format:check` + +## Feature + +* #614: Replaced `path_filters` with `BaseConfig.add_to_excluded_python_paths` and `BaseConfig.excluded_python_paths` diff --git a/doc/github_actions/github_actions.rst b/doc/github_actions/github_actions.rst index 001eeb2ef..e9139e886 100644 --- a/doc/github_actions/github_actions.rst +++ b/doc/github_actions/github_actions.rst @@ -1,6 +1,6 @@ .. _github_actions: -:octicon:`play` Github Actions +:octicon:`play` GitHub Actions =============================== .. toctree:: diff --git a/doc/user_guide/features/formatting_code/index.rst b/doc/user_guide/features/formatting_code/index.rst index 2f769e61e..a720a43a6 100644 --- a/doc/user_guide/features/formatting_code/index.rst +++ b/doc/user_guide/features/formatting_code/index.rst @@ -18,6 +18,13 @@ experience across projects. Nox sessions ++++++++++++ +.. note:: + To prevent Python files from being formatted, you can do one of the following: + * For a single file, use a comment in the files as described in :ref:`this table `. + * If it is a directory (i.e. ``.workspace``), then you can exclude it by + adding it to the ``add_to_excluded_python_paths`` in the project's ``Config`` + defined in the ``noxconfig.py``. + For autoformatting, the following tools are used: * `black `__ - diff --git a/doc/user_guide/features/formatting_code/troubleshooting.rst b/doc/user_guide/features/formatting_code/troubleshooting.rst index 1e9e39ebf..97e26207f 100644 --- a/doc/user_guide/features/formatting_code/troubleshooting.rst +++ b/doc/user_guide/features/formatting_code/troubleshooting.rst @@ -15,6 +15,8 @@ you receive an error from ``format:check`` (i.e. ``isort`` or ``black``), it it likely that you need to update your configuration to align with :ref:`formatting_configuration`. +.. _prevent_auto_format: + The automatic formatting is doing x, but we shouldn't do that because of y --------------------------------------------------------------------------- Usually, automatic formatting is helpful, but there are rare cases where a developer diff --git a/doc/user_guide/migrating.rst b/doc/user_guide/migrating.rst index fcbea6163..1b737b281 100644 --- a/doc/user_guide/migrating.rst +++ b/doc/user_guide/migrating.rst @@ -72,9 +72,9 @@ For example, if test execution isn't performed in the standard way (e.g., :code: # ATTENTION: # In cases where it is reasonable to use "internal" functions, please do those imports # within the function to keep them isolated and simplify future removal or replacement. - from exasol.toolbox.nox._shared import python_files + from exasol.toolbox.nox._shared import get_filtered_python_files - py_files = [f"{file}" for file in python_files(PROJECT_CONFIG.root)] + py_files = get_filtered_python_files(PROJECT_CONFIG.root) print("The original 'format:fix' task has been taken hostage by this overwrite") print("Files:\n{files}".format(files="\n".join(py_files)) diff --git a/exasol/toolbox/config.py b/exasol/toolbox/config.py index d1b68bfb5..8272f1d25 100644 --- a/exasol/toolbox/config.py +++ b/exasol/toolbox/config.py @@ -20,6 +20,16 @@ def valid_version_string(version_string: str) -> str: ValidVersionStr = Annotated[str, AfterValidator(valid_version_string)] +DEFAULT_EXCLUDED_PATHS = { + ".eggs", + ".html-documentation", + ".poetry", + ".sonar", + ".venv", + "dist", + "venv", +} + class BaseConfig(BaseModel): """ @@ -49,6 +59,15 @@ class BaseConfig(BaseModel): default=False, description="If true, creates also the major version tags (v*) automatically", ) + add_to_excluded_python_paths: tuple[str, ...] = Field( + default=(), + description=""" + This is used to extend the default excluded_python_paths. If a more general + path that would be seen in other projects, like .venv, needs to be added into + this argument, please instead modify the + `exasol.toolbox.config.DEFAULT_EXCLUDED_PATHS`. + """, + ) model_config = ConfigDict(frozen=True, arbitrary_types_allowed=True) @computed_field # type: ignore[misc] @@ -65,6 +84,24 @@ def minimum_python_version(self) -> str: index_min_version = versioned.index(min_version) return self.python_versions[index_min_version] + @computed_field # type: ignore[misc] + @property + def excluded_python_paths(self) -> tuple[str, ...]: + """ + There are certain nox sessions: + - lint:code + - lint:security + - lint:typing + - format:fix + - format:check + where it is desired restrict which Python files are considered within the + source_path, like excluding `dist`, `.eggs`. As such, this property is used to + exclude such undesired paths. + """ + return tuple( + DEFAULT_EXCLUDED_PATHS.union(set(self.add_to_excluded_python_paths)) + ) + @computed_field # type: ignore[misc] @property def pyupgrade_argument(self) -> tuple[str, ...]: diff --git a/exasol/toolbox/nox/_format.py b/exasol/toolbox/nox/_format.py index 551ab0ec1..71ac4e477 100644 --- a/exasol/toolbox/nox/_format.py +++ b/exasol/toolbox/nox/_format.py @@ -9,7 +9,7 @@ Mode, _version, check_for_config_attribute, - python_files, + get_filtered_python_files, ) from noxconfig import ( PROJECT_CONFIG, @@ -45,7 +45,7 @@ def command(*args: str) -> Iterable[str]: @nox.session(name="format:fix", python=False) def fix_format(session: Session) -> None: """Runs all automated format fixes on the code base""" - py_files = python_files(PROJECT_CONFIG.root) + py_files = get_filtered_python_files(PROJECT_CONFIG.root) _version(session, Mode.Fix) _pyupgrade(session, config=PROJECT_CONFIG, files=py_files) _ruff(session, mode=Mode.Fix, files=py_files) @@ -55,6 +55,6 @@ def fix_format(session: Session) -> None: @nox.session(name="format:check", python=False) def check_format(session: Session) -> None: """Checks the project for correct formatting""" - py_files = python_files(PROJECT_CONFIG.root) + py_files = get_filtered_python_files(PROJECT_CONFIG.root) _ruff(session, mode=Mode.Check, files=py_files) _code_format(session=session, mode=Mode.Check, files=py_files) diff --git a/exasol/toolbox/nox/_lint.py b/exasol/toolbox/nox/_lint.py index b8d74f201..6ae54eb5e 100644 --- a/exasol/toolbox/nox/_lint.py +++ b/exasol/toolbox/nox/_lint.py @@ -10,16 +10,19 @@ import tomlkit from nox import Session -from exasol.toolbox.nox._shared import python_files +from exasol.toolbox.nox._shared import get_filtered_python_files from exasol.toolbox.util.dependencies.shared_models import PoetryFiles from noxconfig import PROJECT_CONFIG def _pylint(session: Session, files: Iterable[str]) -> None: + json_file = PROJECT_CONFIG.root / ".lint.json" + txt_file = PROJECT_CONFIG.root / ".lint.txt" + session.run( "pylint", "--output-format", - "colorized,json:.lint.json,text:.lint.txt", + f"colorized,json:{json_file},text:{txt_file}", *files, ) @@ -47,7 +50,7 @@ def _security_lint(session: Session, files: Iterable[str]) -> None: "--format", "json", "--output", - ".security.json", + PROJECT_CONFIG.root / ".security.json", "--exit-zero", *files, ) @@ -120,22 +123,22 @@ def report_illegal(illegal: dict[str, list[str]], console: rich.console.Console) @nox.session(name="lint:code", python=False) def lint(session: Session) -> None: """Runs the static code analyzer on the project""" - py_files = python_files(PROJECT_CONFIG.root / PROJECT_CONFIG.source) - _pylint(session, py_files) + py_files = get_filtered_python_files(PROJECT_CONFIG.root / PROJECT_CONFIG.source) + _pylint(session=session, files=py_files) @nox.session(name="lint:typing", python=False) def type_check(session: Session) -> None: """Runs the type checker on the project""" - py_files = [f"{file}" for file in python_files(PROJECT_CONFIG.root)] - _type_check(session, py_files) + py_files = get_filtered_python_files(PROJECT_CONFIG.root) + _type_check(session=session, files=py_files) @nox.session(name="lint:security", python=False) def security_lint(session: Session) -> None: """Runs the security linter on the project""" - py_files = python_files(PROJECT_CONFIG.root / PROJECT_CONFIG.source) - _security_lint(session, py_files) + py_files = get_filtered_python_files(PROJECT_CONFIG.root / PROJECT_CONFIG.source) + _security_lint(session=session, files=py_files) @nox.session(name="lint:dependencies", python=False) diff --git a/exasol/toolbox/nox/_shared.py b/exasol/toolbox/nox/_shared.py index b0d4463ab..fb1989b81 100644 --- a/exasol/toolbox/nox/_shared.py +++ b/exasol/toolbox/nox/_shared.py @@ -3,7 +3,6 @@ import argparse from collections import ChainMap from collections.abc import ( - Iterable, MutableMapping, ) from enum import ( @@ -20,7 +19,6 @@ Config, ) -DEFAULT_PATH_FILTERS = {"dist", ".eggs", "venv", ".poetry"} DOCS_OUTPUT_DIR = ".html-documentation" @@ -40,14 +38,17 @@ class Mode(Enum): Check = auto() -def python_files(project_root: Path) -> Iterable[str]: +def get_filtered_python_files(project_root: Path) -> list[str]: """ - Returns iterable of python files after removing unwanted paths + Returns iterable of Python files after removing excluded paths """ - deny_list = DEFAULT_PATH_FILTERS.union(set(PROJECT_CONFIG.path_filters)) - + check_for_config_attribute(config=PROJECT_CONFIG, attribute="excluded_python_paths") files = project_root.glob("**/*.py") - return [f"{path}" for path in files if not set(path.parts).intersection(deny_list)] + return [ + f"{path}" + for path in files + if not set(path.parts).intersection(PROJECT_CONFIG.excluded_python_paths) + ] def _version(session: Session, mode: Mode) -> None: diff --git a/exasol/toolbox/nox/tasks.py b/exasol/toolbox/nox/tasks.py index 14638c92c..c8475056b 100644 --- a/exasol/toolbox/nox/tasks.py +++ b/exasol/toolbox/nox/tasks.py @@ -34,7 +34,7 @@ def check(session: Session) -> None: """Runs all available checks on the project""" context = _context(session, coverage=True) - py_files = python_files(PROJECT_CONFIG.root) + py_files = get_filtered_python_files(PROJECT_CONFIG.root) _version(session, Mode.Check) _code_format(session, Mode.Check, py_files) _pylint(session, py_files) @@ -65,7 +65,7 @@ def check(session: Session) -> None: Mode, _context, _version, - python_files, + get_filtered_python_files, ) from exasol.toolbox.nox._ci import ( diff --git a/noxconfig.py b/noxconfig.py index a71db3b6e..3f7adc169 100644 --- a/noxconfig.py +++ b/noxconfig.py @@ -56,16 +56,19 @@ class Config(BaseConfig): source: Path = Path("exasol/toolbox") importlinter: Path = Path(__file__).parent / ".import_linter_config" version_file: Path = Path(__file__).parent / "exasol" / "toolbox" / "version.py" - path_filters: Iterable[str] = ( - "metrics-schema", - "project-template", - "idioms", - ".github", - ) plugins: Iterable[object] = (UpdateTemplates,) PROJECT_CONFIG = Config( + add_to_excluded_python_paths=( + # The cookiecutter placeholders do not work well with checks. + # Instead, the format & linting are checked in the + # ``test.integration.project-template``. + "project-template", + # This file comes from poetry (https://install.python-poetry.org/), + # so we should not modify it. + "get_poetry.py", + ), create_major_version_tags=True, # The PTB does not have integration tests run with an Exasol DB, # so for running in the CI, we take the first element. diff --git a/project-template/{{cookiecutter.repo_name}}/noxconfig.py b/project-template/{{cookiecutter.repo_name}}/noxconfig.py index cb7c86200..301a6a5a0 100644 --- a/project-template/{{cookiecutter.repo_name}}/noxconfig.py +++ b/project-template/{{cookiecutter.repo_name}}/noxconfig.py @@ -16,7 +16,6 @@ class Config(BaseConfig): / "{{cookiecutter.package_name}}" / "version.py" ) - path_filters: Iterable[str] = () plugins: Iterable[object] = () PROJECT_CONFIG = Config() diff --git a/test/unit/config_test.py b/test/unit/config_test.py index f65ff03a9..685e7c14e 100644 --- a/test/unit/config_test.py +++ b/test/unit/config_test.py @@ -2,6 +2,7 @@ from pydantic_core._pydantic_core import ValidationError from exasol.toolbox.config import ( + DEFAULT_EXCLUDED_PATHS, BaseConfig, valid_version_string, ) @@ -66,3 +67,22 @@ def test_minimum_python_version(): def test_pyupgrade_argument(minimum_python_version): conf = BaseConfig(python_versions=("3.11", minimum_python_version, "3.12")) assert conf.pyupgrade_argument == ("--py310-plus",) + + +@pytest.mark.parametrize( + "add_to_excluded_python_paths,expected", + [ + pytest.param((), tuple(DEFAULT_EXCLUDED_PATHS), id="no_additions"), + pytest.param( + (next(iter(DEFAULT_EXCLUDED_PATHS)),), + tuple(DEFAULT_EXCLUDED_PATHS), + id="duplicate_addition", + ), + pytest.param( + ("dummy",), tuple(DEFAULT_EXCLUDED_PATHS) + ("dummy",), id="add_a_new_entry" + ), + ], +) +def test_excluded_python_paths(add_to_excluded_python_paths, expected): + conf = BaseConfig(add_to_excluded_python_paths=add_to_excluded_python_paths) + assert sorted(conf.excluded_python_paths) == sorted(expected) diff --git a/test/unit/nox/_lint_test.py b/test/unit/nox/_lint_test.py new file mode 100644 index 000000000..9cd5084c4 --- /dev/null +++ b/test/unit/nox/_lint_test.py @@ -0,0 +1,99 @@ +import json +from inspect import cleandoc +from pathlib import Path +from unittest.mock import patch + +import pytest +from nox.command import CommandFailed + +from exasol.toolbox.nox._lint import ( + lint, + security_lint, + type_check, +) + + +@pytest.fixture +def file_with_multiple_problems(tmp_path): + """ + In this file with multiple problems, it is expected that the nox + lint sessions would detect the following errors: + + * lint:code + * C0304: Final newline missing (missing-final-newline) + * C0114: Missing module docstring (missing-module-docstring) + * W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check) + * lint:typing + * Incompatible types in assignment (expression has type "int", variable has type "str") [assignment] + * lint:security + * [B404:blacklist] Consider possible security implications associated with the subprocess module. + * [B607:start_process_with_partial_path] Starting a process with a partial executable path + * [B603:subprocess_without_shell_equals_true] subprocess call - check for execution of untrusted input. + """ + + file_path = tmp_path / "dummy_file.py" + text = """ + import subprocess + + x: str = 2 + subprocess.run("ls") + """ + file_path.write_text(cleandoc(text)) + return file_path + + +def test_lint(nox_session, tmp_path, file_with_multiple_problems): + with patch("exasol.toolbox.nox._lint.PROJECT_CONFIG") as config: + config.root = tmp_path + config.source = Path("") + with pytest.raises(CommandFailed, match="Returned code 20"): + lint(session=nox_session) + + json_file = tmp_path / ".lint.json" + txt_file = tmp_path / ".lint.txt" + + assert json_file.exists() + assert txt_file.exists() + + contents = json_file.read_text() + errors = {row["message-id"] for row in json.loads(contents)} + assert {"C0114", "C0304", "W1510"}.issubset(errors) + + +def test_type_check(nox_session, tmp_path, file_with_multiple_problems, caplog): + with patch("exasol.toolbox.nox._lint.PROJECT_CONFIG") as config: + config.root = tmp_path + config.source = Path("") + with pytest.raises(CommandFailed, match="Returned code 1"): + type_check(session=nox_session) + + assert caplog.messages[1] == ( + "Command mypy --explicit-package-bases --namespace-packages --show-error-codes " + "--pretty --show-column-numbers --show-error-context --scripts-are-modules " + f"{file_with_multiple_problems} failed with exit code 1" + ) + + +def test_security_lint(nox_session, tmp_path, file_with_multiple_problems): + with patch("exasol.toolbox.nox._lint.PROJECT_CONFIG") as config: + config.root = tmp_path + config.source = Path("") + security_lint(session=nox_session) + + output_file = tmp_path / ".security.json" + assert output_file.exists() + + contents = output_file.read_text() + assert json.loads(contents)["metrics"]["_totals"] == { + "CONFIDENCE.HIGH": 3, + "CONFIDENCE.LOW": 0, + "CONFIDENCE.MEDIUM": 0, + "CONFIDENCE.UNDEFINED": 0, + "SEVERITY.HIGH": 0, + "SEVERITY.LOW": 3, + "SEVERITY.MEDIUM": 0, + "SEVERITY.UNDEFINED": 0, + "loc": 3, + "nosec": 0, + "skipped_tests": 0, + } diff --git a/test/unit/nox/_shared_test.py b/test/unit/nox/_shared_test.py index 374e40afe..8783ad399 100644 --- a/test/unit/nox/_shared_test.py +++ b/test/unit/nox/_shared_test.py @@ -1,15 +1,14 @@ from collections.abc import Iterable from dataclasses import dataclass from pathlib import Path +from unittest.mock import patch import pytest -import noxconfig from exasol.toolbox.config import BaseConfig from exasol.toolbox.nox._shared import ( - DEFAULT_PATH_FILTERS, check_for_config_attribute, - python_files, + get_filtered_python_files, ) @@ -24,13 +23,15 @@ def tmp_directory(tmp_path_factory): @pytest.fixture(scope="session") -def path_filter_directory(): - return "path_filter" +def excluded_python_path(): + return "excluded_python_path" @pytest.fixture(scope="session") -def directories(package_directory, path_filter_directory): - yield DEFAULT_PATH_FILTERS.union({package_directory, path_filter_directory}) +def directories(package_directory, excluded_python_path): + yield set(BaseConfig().excluded_python_paths).union( + {package_directory, excluded_python_path} + ) @pytest.fixture(scope="session") @@ -47,15 +48,14 @@ def create_files(tmp_directory, directories): yield file_list -def test_python_files( - tmp_directory, create_files, package_directory, path_filter_directory +def test_get_filtered_python_files( + tmp_directory, create_files, package_directory, excluded_python_path ): - # Use builtin object to modify attribute path_filters of frozen dataclass instance. - object.__setattr__( - noxconfig.PROJECT_CONFIG, "path_filters", (path_filter_directory,) - ) + config = BaseConfig(add_to_excluded_python_paths=(excluded_python_path,)) + + with patch("exasol.toolbox.nox._shared.PROJECT_CONFIG", config): + actual = get_filtered_python_files(tmp_directory) - actual = python_files(tmp_directory) assert len(actual) == 1 assert "toolbox-dummy" in actual[0] diff --git a/test/unit/project_test.py b/test/unit/project_test.py deleted file mode 100644 index 678272b17..000000000 --- a/test/unit/project_test.py +++ /dev/null @@ -1,10 +0,0 @@ -def test_python_files_with_default_filter() -> None: - pass - - -def test_python_files_with_no_filter() -> None: - pass - - -def test_deny_filter() -> None: - pass