From 7736f7f4404fd907e38d962f85c2ee2c1e9caec9 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 11:02:20 +0200 Subject: [PATCH 01/29] Support configuration using pyproject.toml --- src/dbt_score/cli.py | 6 ++- src/dbt_score/config_parser.py | 72 ++++++++++++++++++++++++++++++++++ src/dbt_score/evaluation.py | 6 +-- src/dbt_score/lint.py | 5 ++- src/dbt_score/rule.py | 42 +++++++++++++++++++- src/dbt_score/rule_registry.py | 31 ++++++++++----- 6 files changed, 145 insertions(+), 17 deletions(-) create mode 100644 src/dbt_score/config_parser.py diff --git a/src/dbt_score/cli.py b/src/dbt_score/cli.py index 4ace740..bdd5d69 100644 --- a/src/dbt_score/cli.py +++ b/src/dbt_score/cli.py @@ -7,6 +7,7 @@ from click.core import ParameterSource from dbt.cli.options import MultiOption +from dbt_score.config_parser import DbtScoreConfig from dbt_score.lint import lint_dbt_project from dbt_score.parse import dbt_parse, get_default_manifest_path @@ -57,7 +58,10 @@ def lint(select: tuple[str], manifest: Path, run_dbt_parse: bool) -> None: if manifest_provided and run_dbt_parse: raise click.UsageError("--run-dbt-parse cannot be used with --manifest.") + config = DbtScoreConfig() + config.load_toml_file() + if run_dbt_parse: dbt_parse() - lint_dbt_project(manifest) + lint_dbt_project(manifest, config) diff --git a/src/dbt_score/config_parser.py b/src/dbt_score/config_parser.py new file mode 100644 index 0000000..2a50580 --- /dev/null +++ b/src/dbt_score/config_parser.py @@ -0,0 +1,72 @@ +"""This module is responsible for parsing the config file.""" + +import configparser +import json +import logging +from dataclasses import dataclass, field +from typing import Any, ClassVar + +logger = logging.getLogger(__name__) + +CONFIG_FILE = "pyproject.toml" + + +@dataclass +class RuleConfig: + """Rule config.""" + severity: int | None = None + description: str | None = None + params: dict[str, Any] = field(default_factory=dict) + + @staticmethod + def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": + """Create a RuleConfig from a dictionary.""" + severity = rule_config.pop("severity", None) + description = rule_config.pop("description", None) + + return RuleConfig(severity=severity, description=description, + params=rule_config) + + +class DbtScoreConfig: + """Dbt score config.""" + + _main_section = "tool.dbt-score" + _options: ClassVar[list[str]] = ["rule_namespaces", "disabled_rules"] + _rules_section = f"{_main_section}.rules" + + def __init__(self) -> None: + """Initialize the DbtScoreConfig object.""" + self.rule_namespaces: list[str] = ["dbt_score_rules"] + self.disabled_rules: list[str] = [] + self.rules_config: dict[str, RuleConfig] = {} + + def set_option(self, option: str, value: Any) -> None: + """Set an option in the config.""" + setattr(self, option, value) + + def load_toml_file(self, file: str = CONFIG_FILE) -> None: + """Load the options from a TOML file.""" + config = configparser.ConfigParser() + config.read(file) + + # Main configuration + if config.has_section(self._main_section): + for option in config.options(self._main_section): + if option in self._options: + self.set_option(option, + json.loads(config.get(self._main_section, option))) + else: + logger.warning( + f"Option {option} in {self._main_section} not supported.") + + # Rule configuration + rules_sections = list( + filter(lambda section: section.startswith(self._rules_section), + config.sections())) + + for rule_section in rules_sections: + rule_name = rule_section.replace(f"{self._rules_section}.", "") + rule_config = {param: json.loads(val) for param, val in + config.items(rule_section)} + self.rules_config[rule_name] = RuleConfig.from_dict(rule_config) diff --git a/src/dbt_score/evaluation.py b/src/dbt_score/evaluation.py index 3630a4d..4790395 100644 --- a/src/dbt_score/evaluation.py +++ b/src/dbt_score/evaluation.py @@ -51,15 +51,13 @@ def __init__( def evaluate(self) -> None: """Evaluate all rules.""" - # Instantiate all rules. In case they keep state across calls, this must be - # done only once. - rules = [rule_class() for rule_class in self._rule_registry.rules.values()] + rules = self._rule_registry.rules.values() for model in self._manifest_loader.models: self.results[model] = {} for rule in rules: try: - result: RuleViolation | None = rule.evaluate(model) + result: RuleViolation | None = rule.evaluate(model, **rule.params) except Exception as e: self.results[model][rule.__class__] = e else: diff --git a/src/dbt_score/lint.py b/src/dbt_score/lint.py index 6003f26..ebc53b9 100644 --- a/src/dbt_score/lint.py +++ b/src/dbt_score/lint.py @@ -2,6 +2,7 @@ from pathlib import Path +from dbt_score.config_parser import DbtScoreConfig from dbt_score.evaluation import Evaluation from dbt_score.formatters.human_readable_formatter import HumanReadableFormatter from dbt_score.models import ManifestLoader @@ -9,12 +10,12 @@ from dbt_score.scoring import Scorer -def lint_dbt_project(manifest_path: Path) -> None: +def lint_dbt_project(manifest_path: Path, config: DbtScoreConfig) -> None: """Lint dbt manifest.""" if not manifest_path.exists(): raise FileNotFoundError(f"Manifest not found at {manifest_path}.") - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(config) rule_registry.load_all() manifest_loader = ManifestLoader(manifest_path) diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 76cc5b1..5fcee58 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -1,9 +1,10 @@ """Rule definitions.""" - +import inspect from dataclasses import dataclass from enum import Enum from typing import Any, Callable, Type, TypeAlias, overload +from dbt_score.config_parser import RuleConfig from dbt_score.models import Model @@ -31,6 +32,11 @@ class Rule: description: str severity: Severity = Severity.MEDIUM + _default_params: dict[str, Any] + + def __init__(self, rule_config: RuleConfig) -> None: + """Initialize the rule.""" + self.params = self.process_config(rule_config) def __init_subclass__(cls, **kwargs) -> None: # type: ignore """Initializes the subclass.""" @@ -38,10 +44,38 @@ def __init_subclass__(cls, **kwargs) -> None: # type: ignore if not hasattr(cls, "description"): raise AttributeError("Subclass must define class attribute `description`.") + def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: + """Process the rule config.""" + rule_params = self._default_params.copy() + + # Overwrite default rule params + for k, v in rule_config.params.items(): + if k in self._default_params: + rule_params[k] = v + else: + raise AttributeError(f"Unknown rule parameter: {k}.") + + self.set_severity(rule_config.severity or self.severity) + self.set_description(rule_config.description or self.description) + + return rule_params + def evaluate(self, model: Model) -> RuleViolation | None: """Evaluates the rule.""" raise NotImplementedError("Subclass must implement method `evaluate`.") + @classmethod + def set_severity(cls, severity: int | Severity) -> None: + """Set the severity of the rule.""" + if isinstance(severity, int): + severity = Severity(severity) + cls.severity = severity + + @classmethod + def set_description(cls, description: str) -> None: + """Set the description of the rule.""" + cls.description = description + @classmethod def source(cls) -> str: """Return the source of the rule, i.e. a fully qualified name.""" @@ -106,6 +140,11 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: """Wrap func to add `self`.""" return func(*args, **kwargs) + # Get default parameters from the rule definition + _default_params = {key: val.default for key, val in + inspect.signature(func).parameters.items() + if val.default != inspect.Parameter.empty} + # Create the rule class inheriting from Rule rule_class = type( func.__name__, @@ -113,6 +152,7 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: { "description": rule_description, "severity": severity, + "_default_params": _default_params, "evaluate": wrapped_func, # Forward origin of the decorated function "__qualname__": func.__qualname__, # https://peps.python.org/pep-3155/ diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index 8e001ff..bfc45e8 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -8,25 +8,34 @@ import pkgutil from typing import Iterator, Type +from dbt_score.config_parser import DbtScoreConfig, RuleConfig from dbt_score.exceptions import DuplicatedRuleException from dbt_score.rule import Rule logger = logging.getLogger(__name__) -THIRD_PARTY_RULES_NAMESPACE = "dbt_score_rules" - class RuleRegistry: """A container for configured rules.""" - def __init__(self) -> None: + def __init__(self, config: DbtScoreConfig) -> None: """Instantiate a rule registry.""" + self.config = config self._rules: dict[str, Type[Rule]] = {} + self._initialized_rules: dict[str, Rule] = {} + + def init_rules(self) -> None: + """Initialize rules.""" + for rule_name, rule_class in self._rules.items(): + rule_config = self.config.rules_config.get(rule_name, + RuleConfig()) + self._initialized_rules[rule_name] = rule_class( + rule_config=rule_config) @property - def rules(self) -> dict[str, Type[Rule]]: + def rules(self) -> dict[str, Rule]: """Get all rules.""" - return self._rules + return self._initialized_rules def _walk_packages(self, namespace_name: str) -> Iterator[str]: """Walk packages and sub-packages recursively.""" @@ -50,14 +59,18 @@ def _load(self, namespace_name: str) -> None: for obj_name in dir(module): obj = module.__dict__[obj_name] if type(obj) is type and issubclass(obj, Rule) and obj is not Rule: - self._add_rule(obj_name, obj) + self._add_rule(f"{module_name}.{obj_name}", obj) def _add_rule(self, name: str, rule: Type[Rule]) -> None: - if name in self.rules: + """Add a rule.""" + if name in self._rules: raise DuplicatedRuleException(name) - self._rules[name] = rule + if name not in self.config.disabled_rules: + self._rules[name] = rule def load_all(self) -> None: """Load all rules, core and third-party.""" self._load("dbt_score.rules") - self._load(THIRD_PARTY_RULES_NAMESPACE) + for namespace in self.config.rule_namespaces: + self._load(namespace) + self.init_rules() From 376c6bc4510dfd1a309023672ad4ea5c920263e5 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 11:19:17 +0200 Subject: [PATCH 02/29] Formatting --- src/dbt_score/config_parser.py | 26 +++++++++++++++++--------- src/dbt_score/rule.py | 8 +++++--- src/dbt_score/rule_registry.py | 6 ++---- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/dbt_score/config_parser.py b/src/dbt_score/config_parser.py index 2a50580..43c9b55 100644 --- a/src/dbt_score/config_parser.py +++ b/src/dbt_score/config_parser.py @@ -14,6 +14,7 @@ @dataclass class RuleConfig: """Rule config.""" + severity: int | None = None description: str | None = None params: dict[str, Any] = field(default_factory=dict) @@ -24,8 +25,9 @@ def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": severity = rule_config.pop("severity", None) description = rule_config.pop("description", None) - return RuleConfig(severity=severity, description=description, - params=rule_config) + return RuleConfig( + severity=severity, description=description, params=rule_config + ) class DbtScoreConfig: @@ -54,19 +56,25 @@ def load_toml_file(self, file: str = CONFIG_FILE) -> None: if config.has_section(self._main_section): for option in config.options(self._main_section): if option in self._options: - self.set_option(option, - json.loads(config.get(self._main_section, option))) + self.set_option( + option, json.loads(config.get(self._main_section, option)) + ) else: logger.warning( - f"Option {option} in {self._main_section} not supported.") + f"Option {option} in {self._main_section} not supported." + ) # Rule configuration rules_sections = list( - filter(lambda section: section.startswith(self._rules_section), - config.sections())) + filter( + lambda section: section.startswith(self._rules_section), + config.sections(), + ) + ) for rule_section in rules_sections: rule_name = rule_section.replace(f"{self._rules_section}.", "") - rule_config = {param: json.loads(val) for param, val in - config.items(rule_section)} + rule_config = { + param: json.loads(val) for param, val in config.items(rule_section) + } self.rules_config[rule_name] = RuleConfig.from_dict(rule_config) diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 5fcee58..b36a4b1 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -141,9 +141,11 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: return func(*args, **kwargs) # Get default parameters from the rule definition - _default_params = {key: val.default for key, val in - inspect.signature(func).parameters.items() - if val.default != inspect.Parameter.empty} + _default_params = { + key: val.default + for key, val in inspect.signature(func).parameters.items() + if val.default != inspect.Parameter.empty + } # Create the rule class inheriting from Rule rule_class = type( diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index bfc45e8..18067dc 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -27,10 +27,8 @@ def __init__(self, config: DbtScoreConfig) -> None: def init_rules(self) -> None: """Initialize rules.""" for rule_name, rule_class in self._rules.items(): - rule_config = self.config.rules_config.get(rule_name, - RuleConfig()) - self._initialized_rules[rule_name] = rule_class( - rule_config=rule_config) + rule_config = self.config.rules_config.get(rule_name, RuleConfig()) + self._initialized_rules[rule_name] = rule_class(rule_config=rule_config) @property def rules(self) -> dict[str, Rule]: From c769daa8b9392e8277407a0627d63178a409a7eb Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 12:18:41 +0200 Subject: [PATCH 03/29] Fix existing tests --- pyproject.toml | 3 ++- src/dbt_score/rule.py | 11 ++++++----- src/dbt_score/rule_registry.py | 4 ++-- tests/conftest.py | 29 +++++++++++++++++++++++++++++ tests/test_cli.py | 27 ++++++++++++++++----------- tests/test_evaluation.py | 2 ++ tests/test_lint.py | 4 ++-- tests/test_rule.py | 13 +++++++------ tests/test_rule_registry.py | 5 ++++- 9 files changed, 70 insertions(+), 28 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c910d2d..74e84bd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,7 +92,8 @@ max-args = 6 [tool.ruff.lint.per-file-ignores] "tests/**/*.py" = [ - "PLR2004", # magic value comparisons + "PLR2004", # Magic value comparisons + "PLR0913" # Too many arguments in function definition ] ### Coverage ### diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index b36a4b1..855fe72 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -1,5 +1,6 @@ """Rule definitions.""" import inspect +import typing from dataclasses import dataclass from enum import Enum from typing import Any, Callable, Type, TypeAlias, overload @@ -32,7 +33,7 @@ class Rule: description: str severity: Severity = Severity.MEDIUM - _default_params: dict[str, Any] + default_params: typing.ClassVar[dict[str, Any]] = {} def __init__(self, rule_config: RuleConfig) -> None: """Initialize the rule.""" @@ -46,11 +47,11 @@ def __init_subclass__(cls, **kwargs) -> None: # type: ignore def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: """Process the rule config.""" - rule_params = self._default_params.copy() + rule_params = self.default_params.copy() # Overwrite default rule params for k, v in rule_config.params.items(): - if k in self._default_params: + if k in self.default_params: rule_params[k] = v else: raise AttributeError(f"Unknown rule parameter: {k}.") @@ -141,7 +142,7 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: return func(*args, **kwargs) # Get default parameters from the rule definition - _default_params = { + default_params = { key: val.default for key, val in inspect.signature(func).parameters.items() if val.default != inspect.Parameter.empty @@ -154,7 +155,7 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: { "description": rule_description, "severity": severity, - "_default_params": _default_params, + "default_params": default_params, "evaluate": wrapped_func, # Forward origin of the decorated function "__qualname__": func.__qualname__, # https://peps.python.org/pep-3155/ diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index 18067dc..fd739ff 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -18,9 +18,9 @@ class RuleRegistry: """A container for configured rules.""" - def __init__(self, config: DbtScoreConfig) -> None: + def __init__(self, config: DbtScoreConfig | None = None) -> None: """Instantiate a rule registry.""" - self.config = config + self.config = config or DbtScoreConfig() self._rules: dict[str, Type[Rule]] = {} self._initialized_rules: dict[str, Rule] = {} diff --git a/tests/conftest.py b/tests/conftest.py index d596db5..a2ca230 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,10 +4,26 @@ from pathlib import Path from typing import Any, Type +from dbt_score.config_parser import DbtScoreConfig, RuleConfig from dbt_score.models import Model from dbt_score.rule import Rule, RuleViolation, Severity, rule from pytest import fixture +# Configuration + + +@fixture +def default_config() -> DbtScoreConfig: + """Return a DbtScoreConfig object.""" + return DbtScoreConfig() + + +@fixture +def default_rule_config() -> RuleConfig: + """Return an empty RuleConfig object.""" + return RuleConfig() + + # Manifest @@ -157,6 +173,19 @@ def rule_severity_critical(model: Model) -> RuleViolation | None: return rule_severity_critical +@fixture +def rule_with_params() -> Type[Rule]: + """An example rule with additional input params.""" + + @rule + def rule_with_params(model: Model, foo: str = "bar") -> RuleViolation | None: + """Rule with CRITICAL severity.""" + if model.name != "model1": + return RuleViolation(message=foo) + + return rule_with_params + + @fixture def rule_error() -> Type[Rule]: """An example rule which fails to run.""" diff --git a/tests/test_cli.py b/tests/test_cli.py index 0146f04..c12a644 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,4 +1,5 @@ """Test the CLI.""" +from unittest.mock import patch import pytest from click.testing import CliRunner @@ -8,17 +9,19 @@ def test_invalid_options(): """Test invalid cli options.""" runner = CliRunner() - result = runner.invoke( - lint, ["--manifest", "fake_manifest.json", "--run-dbt-parse"] - ) - assert result.exit_code == 2 # pylint: disable=PLR2004 + with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + result = runner.invoke( + lint, ["--manifest", "fake_manifest.json", "--run-dbt-parse"] + ) + assert result.exit_code == 2 # pylint: disable=PLR2004 def test_lint_existing_manifest(manifest_path): """Test lint with an existing manifest.""" - runner = CliRunner() - result = runner.invoke(lint, ["--manifest", manifest_path]) - assert result.exit_code == 0 + with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + runner = CliRunner() + result = runner.invoke(lint, ["--manifest", manifest_path]) + assert result.exit_code == 0 def test_lint_non_existing_manifest(): @@ -27,10 +30,12 @@ def test_lint_non_existing_manifest(): # Provide manifest in command line with pytest.raises(FileNotFoundError): - runner.invoke( - lint, ["--manifest", "fake_manifest.json"], catch_exceptions=False - ) + with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + runner.invoke( + lint, ["--manifest", "fake_manifest.json"], catch_exceptions=False + ) # Use default manifest path with pytest.raises(FileNotFoundError): - runner.invoke(lint, catch_exceptions=False) + with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + runner.invoke(lint, catch_exceptions=False) diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index c03981f..2958257 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -25,6 +25,7 @@ def test_evaluation_low_medium_high( rule_registry._add_rule("rule_severity_medium", rule_severity_medium) rule_registry._add_rule("rule_severity_high", rule_severity_high) rule_registry._add_rule("rule_error", rule_error) + rule_registry.init_rules() evaluation = Evaluation( rule_registry=rule_registry, @@ -65,6 +66,7 @@ def test_evaluation_critical( rule_registry = RuleRegistry() rule_registry._add_rule("rule_severity_low", rule_severity_low) rule_registry._add_rule("rule_severity_critical", rule_severity_critical) + rule_registry.init_rules() evaluation = Evaluation( rule_registry=rule_registry, diff --git a/tests/test_lint.py b/tests/test_lint.py index ed904b9..d616259 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -7,11 +7,11 @@ @patch("dbt_score.lint.Evaluation") -def test_lint_dbt_project(mock_evaluation, manifest_path): +def test_lint_dbt_project(mock_evaluation, manifest_path, default_config): """Test linting the dbt project.""" # Instance of classes are the same Mocks mock_evaluation.return_value = mock_evaluation - lint_dbt_project(manifest_path) + lint_dbt_project(manifest_path, default_config) mock_evaluation.evaluate.assert_called_once() diff --git a/tests/test_rule.py b/tests/test_rule.py index 9819e75..0ba0991 100644 --- a/tests/test_rule.py +++ b/tests/test_rule.py @@ -12,12 +12,13 @@ def test_rule_decorator_and_class( class_rule, model1, model2, + default_rule_config, ): """Test rule creation with the rule decorator and class.""" - decorator_rule_instance = decorator_rule() - decorator_rule_no_parens_instance = decorator_rule_no_parens() - decorator_rule_args_instance = decorator_rule_args() - class_rule_instance = class_rule() + decorator_rule_instance = decorator_rule(default_rule_config) + decorator_rule_no_parens_instance = decorator_rule_no_parens(default_rule_config) + decorator_rule_args_instance = decorator_rule_args(default_rule_config) + class_rule_instance = class_rule(default_rule_config) def assertions(rule_instance): assert isinstance(rule_instance, Rule) @@ -55,7 +56,7 @@ def evaluate(self, model: Model) -> RuleViolation | None: return None -def test_missing_evaluate_rule_class(model1): +def test_missing_evaluate_rule_class(model1, default_rule_config): """Test missing evaluate implementation in rule class.""" class BadRule(Rule): @@ -63,7 +64,7 @@ class BadRule(Rule): description = "Description of the rule." - rule = BadRule() + rule = BadRule(rule_config=default_rule_config) with pytest.raises(NotImplementedError): rule.evaluate(model1) diff --git a/tests/test_rule_registry.py b/tests/test_rule_registry.py index 0d90166..9401832 100644 --- a/tests/test_rule_registry.py +++ b/tests/test_rule_registry.py @@ -9,7 +9,10 @@ def test_rule_registry_discovery(): """Ensure rules can be found in a given namespace recursively.""" r = RuleRegistry() r._load("tests.rules") - assert sorted(r.rules.keys()) == ["rule_test_example", "rule_test_nested_example"] + assert sorted(r._rules.keys()) == [ + "tests.rules.example.rule_test_example", + "tests.rules.nested.example.rule_test_nested_example", + ] def test_rule_registry_no_duplicates(): From 0d278bf772e154f2577aa2f3da357c8d92823688 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 12:24:29 +0200 Subject: [PATCH 04/29] Add check for pyproject.toml --- src/dbt_score/cli.py | 5 +++-- src/dbt_score/config_parser.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/dbt_score/cli.py b/src/dbt_score/cli.py index bdd5d69..a2e940e 100644 --- a/src/dbt_score/cli.py +++ b/src/dbt_score/cli.py @@ -7,7 +7,7 @@ from click.core import ParameterSource from dbt.cli.options import MultiOption -from dbt_score.config_parser import DbtScoreConfig +from dbt_score.config_parser import DEFAULT_CONFIG_FILE, DbtScoreConfig from dbt_score.lint import lint_dbt_project from dbt_score.parse import dbt_parse, get_default_manifest_path @@ -59,7 +59,8 @@ def lint(select: tuple[str], manifest: Path, run_dbt_parse: bool) -> None: raise click.UsageError("--run-dbt-parse cannot be used with --manifest.") config = DbtScoreConfig() - config.load_toml_file() + if Path(DEFAULT_CONFIG_FILE).exists(): + config.load_toml_file(DEFAULT_CONFIG_FILE) if run_dbt_parse: dbt_parse() diff --git a/src/dbt_score/config_parser.py b/src/dbt_score/config_parser.py index 43c9b55..9d370c0 100644 --- a/src/dbt_score/config_parser.py +++ b/src/dbt_score/config_parser.py @@ -8,7 +8,7 @@ logger = logging.getLogger(__name__) -CONFIG_FILE = "pyproject.toml" +DEFAULT_CONFIG_FILE = "pyproject.toml" @dataclass @@ -47,7 +47,7 @@ def set_option(self, option: str, value: Any) -> None: """Set an option in the config.""" setattr(self, option, value) - def load_toml_file(self, file: str = CONFIG_FILE) -> None: + def load_toml_file(self, file: str) -> None: """Load the options from a TOML file.""" config = configparser.ConfigParser() config.read(file) From 92398fcf72b507aacb80055b47d38a69f3f2402f Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 13:20:21 +0200 Subject: [PATCH 05/29] Make rule_config an optional parameter --- src/dbt_score/rule.py | 4 ++-- tests/test_rule.py | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 855fe72..5bb0a39 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -35,9 +35,9 @@ class Rule: severity: Severity = Severity.MEDIUM default_params: typing.ClassVar[dict[str, Any]] = {} - def __init__(self, rule_config: RuleConfig) -> None: + def __init__(self, rule_config: RuleConfig | None = None) -> None: """Initialize the rule.""" - self.params = self.process_config(rule_config) + self.params = self.process_config(rule_config) if rule_config else {} def __init_subclass__(cls, **kwargs) -> None: # type: ignore """Initializes the subclass.""" diff --git a/tests/test_rule.py b/tests/test_rule.py index 0ba0991..9819e75 100644 --- a/tests/test_rule.py +++ b/tests/test_rule.py @@ -12,13 +12,12 @@ def test_rule_decorator_and_class( class_rule, model1, model2, - default_rule_config, ): """Test rule creation with the rule decorator and class.""" - decorator_rule_instance = decorator_rule(default_rule_config) - decorator_rule_no_parens_instance = decorator_rule_no_parens(default_rule_config) - decorator_rule_args_instance = decorator_rule_args(default_rule_config) - class_rule_instance = class_rule(default_rule_config) + decorator_rule_instance = decorator_rule() + decorator_rule_no_parens_instance = decorator_rule_no_parens() + decorator_rule_args_instance = decorator_rule_args() + class_rule_instance = class_rule() def assertions(rule_instance): assert isinstance(rule_instance, Rule) @@ -56,7 +55,7 @@ def evaluate(self, model: Model) -> RuleViolation | None: return None -def test_missing_evaluate_rule_class(model1, default_rule_config): +def test_missing_evaluate_rule_class(model1): """Test missing evaluate implementation in rule class.""" class BadRule(Rule): @@ -64,7 +63,7 @@ class BadRule(Rule): description = "Description of the rule." - rule = BadRule(rule_config=default_rule_config) + rule = BadRule() with pytest.raises(NotImplementedError): rule.evaluate(model1) From 1ab1c9ed1466146665390200c7a5701d505036e7 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 13:23:29 +0200 Subject: [PATCH 06/29] Make general config an optional parameter --- src/dbt_score/lint.py | 5 ++++- tests/test_lint.py | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/dbt_score/lint.py b/src/dbt_score/lint.py index ebc53b9..ade4ca2 100644 --- a/src/dbt_score/lint.py +++ b/src/dbt_score/lint.py @@ -10,11 +10,14 @@ from dbt_score.scoring import Scorer -def lint_dbt_project(manifest_path: Path, config: DbtScoreConfig) -> None: +def lint_dbt_project(manifest_path: Path, config: DbtScoreConfig | None = None) -> None: """Lint dbt manifest.""" if not manifest_path.exists(): raise FileNotFoundError(f"Manifest not found at {manifest_path}.") + if config is None: + config = DbtScoreConfig() + rule_registry = RuleRegistry(config) rule_registry.load_all() diff --git a/tests/test_lint.py b/tests/test_lint.py index d616259..ed904b9 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -7,11 +7,11 @@ @patch("dbt_score.lint.Evaluation") -def test_lint_dbt_project(mock_evaluation, manifest_path, default_config): +def test_lint_dbt_project(mock_evaluation, manifest_path): """Test linting the dbt project.""" # Instance of classes are the same Mocks mock_evaluation.return_value = mock_evaluation - lint_dbt_project(manifest_path, default_config) + lint_dbt_project(manifest_path) mock_evaluation.evaluate.assert_called_once() From 651fd5311ebfd99a5c9002025ae0f5d96c181b81 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 14:19:52 +0200 Subject: [PATCH 07/29] Add tests --- tests/conftest.py | 22 ++++++++++++--- tests/resources/invalid_pyproject.toml | 2 ++ tests/resources/pyproject.toml | 10 +++++++ tests/test_config.py | 38 ++++++++++++++++++++++++++ tests/test_evaluation.py | 32 ++++++++++++++++++++++ 5 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 tests/resources/invalid_pyproject.toml create mode 100644 tests/resources/pyproject.toml create mode 100644 tests/test_config.py diff --git a/tests/conftest.py b/tests/conftest.py index a2ca230..a1af835 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,6 +12,18 @@ # Configuration +@fixture +def valid_config_path() -> Path: + """Return the path of the configuration.""" + return Path(__file__).parent / "resources" / "pyproject.toml" + + +@fixture +def invalid_config_path() -> Path: + """Return the path of the configuration.""" + return Path(__file__).parent / "resources" / "invalid_pyproject.toml" + + @fixture def default_config() -> DbtScoreConfig: """Return a DbtScoreConfig object.""" @@ -178,10 +190,12 @@ def rule_with_params() -> Type[Rule]: """An example rule with additional input params.""" @rule - def rule_with_params(model: Model, foo: str = "bar") -> RuleViolation | None: - """Rule with CRITICAL severity.""" - if model.name != "model1": - return RuleViolation(message=foo) + def rule_with_params( + model: Model, model_name: str = "model1" + ) -> RuleViolation | None: + """Rule with additional input params.""" + if model.name != model_name: + return RuleViolation(message=model_name) return rule_with_params diff --git a/tests/resources/invalid_pyproject.toml b/tests/resources/invalid_pyproject.toml new file mode 100644 index 0000000..77b5842 --- /dev/null +++ b/tests/resources/invalid_pyproject.toml @@ -0,0 +1,2 @@ +[tool.dbt-score] +foo = "bar" diff --git a/tests/resources/pyproject.toml b/tests/resources/pyproject.toml new file mode 100644 index 0000000..c9f9f15 --- /dev/null +++ b/tests/resources/pyproject.toml @@ -0,0 +1,10 @@ +[tool.dbt-score] +rule_namespaces = ["namespace_foo"] +disabled_rules = ["foo", "bar"] + + +[tool.dbt-score.rules.foobar] +severity=4 + +[tool.dbt-score.rules.rule_with_params] +model_name="model2" diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 0000000..e0ac824 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,38 @@ +"""Tests for the module config_parser.""" + +import pytest +from dbt_score.config_parser import DbtScoreConfig, RuleConfig +from dbt_score.rule import Severity + + +def test_load_valid_toml_file(valid_config_path): + """Test that a valid config file loads correctly.""" + config = DbtScoreConfig() + config.load_toml_file(str(valid_config_path)) + assert config.rule_namespaces == ["namespace_foo"] + assert config.disabled_rules == ["foo", "bar"] + assert config.rules_config["foobar"].severity == 4 + + +def test_load_invalid_toml_file(caplog, invalid_config_path): + """Test that an invalid config file logs a warning.""" + config = DbtScoreConfig() + config.load_toml_file(str(invalid_config_path)) + assert "Option foo in tool.dbt-score not supported." in caplog.text + + +def test_invalid_rule_config(rule_severity_low): + """Test that an invalid rule config raises an exception.""" + config = RuleConfig(params={"foo": "bar"}) + with pytest.raises(AttributeError, match="Unknown rule parameter: foo."): + rule_severity_low(config) + + +def test_valid_rule_config(valid_config_path, rule_with_params): + """Test that a valid rule config can be loaded.""" + config = RuleConfig(severity=4, description="foo", params={"model_name": "baz"}) + rule_with_params = rule_with_params(config) + assert rule_with_params.severity == Severity.CRITICAL + assert rule_with_params.description == "foo" + assert rule_with_params.default_params == {"model_name": "model1"} + assert rule_with_params.params == {"model_name": "baz"} diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index 2958257..8b2796d 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -2,6 +2,7 @@ from unittest.mock import Mock +from dbt_score.config_parser import DbtScoreConfig from dbt_score.evaluation import Evaluation from dbt_score.models import ManifestLoader from dbt_score.rule import RuleViolation @@ -134,3 +135,34 @@ def test_evaluation_no_model_no_rule(manifest_empty_path): assert len(evaluation.results) == 0 assert list(evaluation.scores.values()) == [] + + +def test_evaluation_rule_with_params( + manifest_path, rule_with_params, valid_config_path +): + """Test rule evaluation with parameters.""" + manifest_loader = ManifestLoader(manifest_path) + model1 = manifest_loader.models[0] + model2 = manifest_loader.models[1] + + config = DbtScoreConfig() + config.load_toml_file(str(valid_config_path)) + + rule_registry = RuleRegistry(config) + rule_registry._add_rule("rule_with_params", rule_with_params) + rule_registry.init_rules() + + evaluation = Evaluation( + rule_registry=rule_registry, + manifest_loader=manifest_loader, + formatter=Mock(), + scorer=Mock(), + ) + evaluation.evaluate() + + assert ( + rule_with_params.default_params + != rule_registry.rules["rule_with_params"].params + ) + assert evaluation.results[model1][rule_with_params] is not None + assert evaluation.results[model2][rule_with_params] is None From 3622da32d29d9beff045179e3aedd55ddd110af6 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 14:30:02 +0200 Subject: [PATCH 08/29] Add mkdocs files --- docs/reference/config_parser.md | 3 +++ mkdocs.yml | 1 + 2 files changed, 4 insertions(+) create mode 100644 docs/reference/config_parser.md diff --git a/docs/reference/config_parser.md b/docs/reference/config_parser.md new file mode 100644 index 0000000..55d52ad --- /dev/null +++ b/docs/reference/config_parser.md @@ -0,0 +1,3 @@ +# Config parser + +::: dbt_score.config_parser diff --git a/mkdocs.yml b/mkdocs.yml index b9fcd94..cb34ef4 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -8,6 +8,7 @@ nav: - Home: index.md - Reference: - reference/cli.md + - reference/config_parser.md - reference/exceptions.md - reference/evaluation.md - reference/models.md From a465a29641f26d8ce9a77b28bae4768ff5b38290 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Wed, 1 May 2024 14:39:23 +0200 Subject: [PATCH 09/29] rule-configuration Fix formatting --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 764e5f3..90a4567 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,8 +4,8 @@ from pathlib import Path from typing import Any, Type -from dbt_score.config_parser import DbtScoreConfig, RuleConfig from dbt_score import Model, Rule, RuleViolation, Severity, rule +from dbt_score.config_parser import DbtScoreConfig, RuleConfig from pytest import fixture # Configuration From 933207d046b0e6671aa468755e4a40ed09fe7f43 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Thu, 2 May 2024 08:24:53 +0200 Subject: [PATCH 10/29] Add a test for disabling rules --- tests/test_rule_registry.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_rule_registry.py b/tests/test_rule_registry.py index 9401832..13f1d21 100644 --- a/tests/test_rule_registry.py +++ b/tests/test_rule_registry.py @@ -1,6 +1,7 @@ """Unit tests for the rule registry.""" import pytest +from dbt_score.config_parser import DbtScoreConfig from dbt_score.exceptions import DuplicatedRuleException from dbt_score.rule_registry import RuleRegistry @@ -15,6 +16,17 @@ def test_rule_registry_discovery(): ] +def test_disabled_rule_registry_discovery(): + """Ensure disabled rules are not discovered.""" + config = DbtScoreConfig() + config.disabled_rules = ["tests.rules.nested.example.rule_test_nested_example"] + r = RuleRegistry(config) + r._load("tests.rules") + assert sorted(r._rules.keys()) == [ + "tests.rules.example.rule_test_example", + ] + + def test_rule_registry_no_duplicates(): """Ensure no duplicate rule names can coexist.""" r = RuleRegistry() From dc58c5a767d38de21f6722b87bd40e0d57b3b380 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Thu, 2 May 2024 08:31:58 +0200 Subject: [PATCH 11/29] Improve docstrings --- src/dbt_score/config_parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dbt_score/config_parser.py b/src/dbt_score/config_parser.py index 9d370c0..b893c1e 100644 --- a/src/dbt_score/config_parser.py +++ b/src/dbt_score/config_parser.py @@ -1,4 +1,4 @@ -"""This module is responsible for parsing the config file.""" +"""This module is responsible for parsing configuration.""" import configparser import json @@ -13,7 +13,7 @@ @dataclass class RuleConfig: - """Rule config.""" + """Configuration for a rule.""" severity: int | None = None description: str | None = None @@ -31,7 +31,7 @@ def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": class DbtScoreConfig: - """Dbt score config.""" + """Configuration for dbt-score.""" _main_section = "tool.dbt-score" _options: ClassVar[list[str]] = ["rule_namespaces", "disabled_rules"] From e62836dc512b7824979398b868855b57f5199643 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 08:38:40 +0200 Subject: [PATCH 12/29] Update src/dbt_score/rule.py Co-authored-by: Kirill Druzhinin <112948926+druzhinin-kirill@users.noreply.github.com> --- src/dbt_score/rule.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 5bb0a39..b260008 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -1,4 +1,5 @@ """Rule definitions.""" + import inspect import typing from dataclasses import dataclass From 4603f43c268017e1ca8ee262f4098910d4bde32c Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:29:18 +0200 Subject: [PATCH 13/29] Process review comments --- pyproject.toml | 1 - src/dbt_score/cli.py | 4 +- src/dbt_score/config.py | 67 ++++++++++++++++++++++++++++ src/dbt_score/config_parser.py | 80 ---------------------------------- src/dbt_score/lint.py | 7 +-- src/dbt_score/rule.py | 12 ++--- src/dbt_score/rule_registry.py | 6 +-- tests/conftest.py | 8 ++-- tests/resources/pyproject.toml | 3 ++ tests/test_cli.py | 8 ++-- tests/test_config.py | 10 ++--- tests/test_evaluation.py | 4 +- tests/test_lint.py | 3 +- tests/test_rule_registry.py | 17 +++++++- 14 files changed, 113 insertions(+), 117 deletions(-) create mode 100644 src/dbt_score/config.py delete mode 100644 src/dbt_score/config_parser.py diff --git a/pyproject.toml b/pyproject.toml index cb36185..9aae426 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -93,7 +93,6 @@ max-args = 6 [tool.ruff.lint.per-file-ignores] "tests/**/*.py" = [ "PLR2004", # Magic value comparisons - "PLR0913" # Too many arguments in function definition ] ### Coverage ### diff --git a/src/dbt_score/cli.py b/src/dbt_score/cli.py index a2e940e..b87537a 100644 --- a/src/dbt_score/cli.py +++ b/src/dbt_score/cli.py @@ -7,7 +7,7 @@ from click.core import ParameterSource from dbt.cli.options import MultiOption -from dbt_score.config_parser import DEFAULT_CONFIG_FILE, DbtScoreConfig +from dbt_score.config import DEFAULT_CONFIG_FILE, Config from dbt_score.lint import lint_dbt_project from dbt_score.parse import dbt_parse, get_default_manifest_path @@ -58,7 +58,7 @@ def lint(select: tuple[str], manifest: Path, run_dbt_parse: bool) -> None: if manifest_provided and run_dbt_parse: raise click.UsageError("--run-dbt-parse cannot be used with --manifest.") - config = DbtScoreConfig() + config = Config() if Path(DEFAULT_CONFIG_FILE).exists(): config.load_toml_file(DEFAULT_CONFIG_FILE) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py new file mode 100644 index 0000000..8f03733 --- /dev/null +++ b/src/dbt_score/config.py @@ -0,0 +1,67 @@ +"""This module is responsible for loading configuration.""" + +import logging +from dataclasses import dataclass, field +from typing import Any, ClassVar + +import tomllib + +logger = logging.getLogger(__name__) + +DEFAULT_CONFIG_FILE = "pyproject.toml" + + +@dataclass +class RuleConfig: + """Configuration for a rule.""" + + severity: int | None = None + params: dict[str, Any] = field(default_factory=dict) + + @staticmethod + def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": + """Create a RuleConfig from a dictionary.""" + copy = rule_config.copy() + severity = copy.pop("severity", None) + return RuleConfig(severity=severity, params=copy) + + +class Config: + """Configuration for dbt-score.""" + + _main_section = "tool.dbt-score" + _options: ClassVar[list[str]] = ["rule_namespaces", "disabled_rules"] + _rules_section = f"{_main_section}.rules" + + def __init__(self) -> None: + """Initialize the Config object.""" + self.rule_namespaces: list[str] = ["dbt_score_rules"] + self.disabled_rules: list[str] = [] + self.rules_config: dict[str, RuleConfig] = {} + + def set_option(self, option: str, value: Any) -> None: + """Set an option in the config.""" + setattr(self, option, value) + + def load_toml_file(self, file: str) -> None: + """Load the options from a TOML file.""" + with open(file, "rb") as f: + toml_data = tomllib.load(f) + tools = toml_data.get("tool", {}) + dbt_score_config = tools.get("dbt-score", {}) + + # Main configuration + for option, value in dbt_score_config.items(): + # If value is a dictionary, it's another section + if option in self._options and not isinstance(value, dict): + self.set_option(option, value) + else: + logger.warning( + f"Option {option} in {self._main_section} not supported." + ) + + # Rule configuration + self.rules_config = { + name: RuleConfig.from_dict(config) + for name, config in dbt_score_config.get("rules", {}).items() + } diff --git a/src/dbt_score/config_parser.py b/src/dbt_score/config_parser.py deleted file mode 100644 index b893c1e..0000000 --- a/src/dbt_score/config_parser.py +++ /dev/null @@ -1,80 +0,0 @@ -"""This module is responsible for parsing configuration.""" - -import configparser -import json -import logging -from dataclasses import dataclass, field -from typing import Any, ClassVar - -logger = logging.getLogger(__name__) - -DEFAULT_CONFIG_FILE = "pyproject.toml" - - -@dataclass -class RuleConfig: - """Configuration for a rule.""" - - severity: int | None = None - description: str | None = None - params: dict[str, Any] = field(default_factory=dict) - - @staticmethod - def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": - """Create a RuleConfig from a dictionary.""" - severity = rule_config.pop("severity", None) - description = rule_config.pop("description", None) - - return RuleConfig( - severity=severity, description=description, params=rule_config - ) - - -class DbtScoreConfig: - """Configuration for dbt-score.""" - - _main_section = "tool.dbt-score" - _options: ClassVar[list[str]] = ["rule_namespaces", "disabled_rules"] - _rules_section = f"{_main_section}.rules" - - def __init__(self) -> None: - """Initialize the DbtScoreConfig object.""" - self.rule_namespaces: list[str] = ["dbt_score_rules"] - self.disabled_rules: list[str] = [] - self.rules_config: dict[str, RuleConfig] = {} - - def set_option(self, option: str, value: Any) -> None: - """Set an option in the config.""" - setattr(self, option, value) - - def load_toml_file(self, file: str) -> None: - """Load the options from a TOML file.""" - config = configparser.ConfigParser() - config.read(file) - - # Main configuration - if config.has_section(self._main_section): - for option in config.options(self._main_section): - if option in self._options: - self.set_option( - option, json.loads(config.get(self._main_section, option)) - ) - else: - logger.warning( - f"Option {option} in {self._main_section} not supported." - ) - - # Rule configuration - rules_sections = list( - filter( - lambda section: section.startswith(self._rules_section), - config.sections(), - ) - ) - - for rule_section in rules_sections: - rule_name = rule_section.replace(f"{self._rules_section}.", "") - rule_config = { - param: json.loads(val) for param, val in config.items(rule_section) - } - self.rules_config[rule_name] = RuleConfig.from_dict(rule_config) diff --git a/src/dbt_score/lint.py b/src/dbt_score/lint.py index ade4ca2..5fe1a40 100644 --- a/src/dbt_score/lint.py +++ b/src/dbt_score/lint.py @@ -2,7 +2,7 @@ from pathlib import Path -from dbt_score.config_parser import DbtScoreConfig +from dbt_score.config import Config from dbt_score.evaluation import Evaluation from dbt_score.formatters.human_readable_formatter import HumanReadableFormatter from dbt_score.models import ManifestLoader @@ -10,14 +10,11 @@ from dbt_score.scoring import Scorer -def lint_dbt_project(manifest_path: Path, config: DbtScoreConfig | None = None) -> None: +def lint_dbt_project(manifest_path: Path, config: Config) -> None: """Lint dbt manifest.""" if not manifest_path.exists(): raise FileNotFoundError(f"Manifest not found at {manifest_path}.") - if config is None: - config = DbtScoreConfig() - rule_registry = RuleRegistry(config) rule_registry.load_all() diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index b260008..60ad5f2 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -6,7 +6,7 @@ from enum import Enum from typing import Any, Callable, Type, TypeAlias, overload -from dbt_score.config_parser import RuleConfig +from dbt_score.config import RuleConfig from dbt_score.models import Model @@ -57,8 +57,9 @@ def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: else: raise AttributeError(f"Unknown rule parameter: {k}.") - self.set_severity(rule_config.severity or self.severity) - self.set_description(rule_config.description or self.description) + self.set_severity( + rule_config.severity + ) if rule_config.severity else rule_config.severity return rule_params @@ -73,11 +74,6 @@ def set_severity(cls, severity: int | Severity) -> None: severity = Severity(severity) cls.severity = severity - @classmethod - def set_description(cls, description: str) -> None: - """Set the description of the rule.""" - cls.description = description - @classmethod def source(cls) -> str: """Return the source of the rule, i.e. a fully qualified name.""" diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index fd739ff..44a754f 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -8,7 +8,7 @@ import pkgutil from typing import Iterator, Type -from dbt_score.config_parser import DbtScoreConfig, RuleConfig +from dbt_score.config import Config, RuleConfig from dbt_score.exceptions import DuplicatedRuleException from dbt_score.rule import Rule @@ -18,9 +18,9 @@ class RuleRegistry: """A container for configured rules.""" - def __init__(self, config: DbtScoreConfig | None = None) -> None: + def __init__(self, config: Config | None = None) -> None: """Instantiate a rule registry.""" - self.config = config or DbtScoreConfig() + self.config = config or Config() self._rules: dict[str, Type[Rule]] = {} self._initialized_rules: dict[str, Rule] = {} diff --git a/tests/conftest.py b/tests/conftest.py index 90a4567..376f8ca 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,7 +5,7 @@ from typing import Any, Type from dbt_score import Model, Rule, RuleViolation, Severity, rule -from dbt_score.config_parser import DbtScoreConfig, RuleConfig +from dbt_score.config import Config, RuleConfig from pytest import fixture # Configuration @@ -24,9 +24,9 @@ def invalid_config_path() -> Path: @fixture -def default_config() -> DbtScoreConfig: - """Return a DbtScoreConfig object.""" - return DbtScoreConfig() +def default_config() -> Config: + """Return a Config object.""" + return Config() @fixture diff --git a/tests/resources/pyproject.toml b/tests/resources/pyproject.toml index c9f9f15..e496972 100644 --- a/tests/resources/pyproject.toml +++ b/tests/resources/pyproject.toml @@ -8,3 +8,6 @@ severity=4 [tool.dbt-score.rules.rule_with_params] model_name="model2" + +[tool.dbt-score.rules."tests.rules.example.rule_test_example"] +severity=4 diff --git a/tests/test_cli.py b/tests/test_cli.py index c12a644..e4cdd78 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9,7 +9,7 @@ def test_invalid_options(): """Test invalid cli options.""" runner = CliRunner() - with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + with patch("dbt_score.cli.Config.load_toml_file"): result = runner.invoke( lint, ["--manifest", "fake_manifest.json", "--run-dbt-parse"] ) @@ -18,7 +18,7 @@ def test_invalid_options(): def test_lint_existing_manifest(manifest_path): """Test lint with an existing manifest.""" - with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + with patch("dbt_score.cli.Config.load_toml_file"): runner = CliRunner() result = runner.invoke(lint, ["--manifest", manifest_path]) assert result.exit_code == 0 @@ -30,12 +30,12 @@ def test_lint_non_existing_manifest(): # Provide manifest in command line with pytest.raises(FileNotFoundError): - with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + with patch("dbt_score.cli.Config.load_toml_file"): runner.invoke( lint, ["--manifest", "fake_manifest.json"], catch_exceptions=False ) # Use default manifest path with pytest.raises(FileNotFoundError): - with patch("dbt_score.cli.DbtScoreConfig.load_toml_file"): + with patch("dbt_score.cli.Config.load_toml_file"): runner.invoke(lint, catch_exceptions=False) diff --git a/tests/test_config.py b/tests/test_config.py index e0ac824..4afda8b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,22 +1,23 @@ """Tests for the module config_parser.""" import pytest -from dbt_score.config_parser import DbtScoreConfig, RuleConfig +from dbt_score.config import Config, RuleConfig from dbt_score.rule import Severity def test_load_valid_toml_file(valid_config_path): """Test that a valid config file loads correctly.""" - config = DbtScoreConfig() + config = Config() config.load_toml_file(str(valid_config_path)) assert config.rule_namespaces == ["namespace_foo"] assert config.disabled_rules == ["foo", "bar"] assert config.rules_config["foobar"].severity == 4 + assert config.rules_config["tests.rules.example.rule_test_example"].severity == 4 def test_load_invalid_toml_file(caplog, invalid_config_path): """Test that an invalid config file logs a warning.""" - config = DbtScoreConfig() + config = Config() config.load_toml_file(str(invalid_config_path)) assert "Option foo in tool.dbt-score not supported." in caplog.text @@ -30,9 +31,8 @@ def test_invalid_rule_config(rule_severity_low): def test_valid_rule_config(valid_config_path, rule_with_params): """Test that a valid rule config can be loaded.""" - config = RuleConfig(severity=4, description="foo", params={"model_name": "baz"}) + config = RuleConfig(severity=4, params={"model_name": "baz"}) rule_with_params = rule_with_params(config) assert rule_with_params.severity == Severity.CRITICAL - assert rule_with_params.description == "foo" assert rule_with_params.default_params == {"model_name": "model1"} assert rule_with_params.params == {"model_name": "baz"} diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index 8b2796d..d3ac1df 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -2,7 +2,7 @@ from unittest.mock import Mock -from dbt_score.config_parser import DbtScoreConfig +from dbt_score.config import Config from dbt_score.evaluation import Evaluation from dbt_score.models import ManifestLoader from dbt_score.rule import RuleViolation @@ -145,7 +145,7 @@ def test_evaluation_rule_with_params( model1 = manifest_loader.models[0] model2 = manifest_loader.models[1] - config = DbtScoreConfig() + config = Config() config.load_toml_file(str(valid_config_path)) rule_registry = RuleRegistry(config) diff --git a/tests/test_lint.py b/tests/test_lint.py index ed904b9..41243e2 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -3,6 +3,7 @@ from unittest.mock import patch +from dbt_score.config import Config from dbt_score.lint import lint_dbt_project @@ -12,6 +13,6 @@ def test_lint_dbt_project(mock_evaluation, manifest_path): # Instance of classes are the same Mocks mock_evaluation.return_value = mock_evaluation - lint_dbt_project(manifest_path) + lint_dbt_project(manifest_path, Config()) mock_evaluation.evaluate.assert_called_once() diff --git a/tests/test_rule_registry.py b/tests/test_rule_registry.py index 13f1d21..ac7a98d 100644 --- a/tests/test_rule_registry.py +++ b/tests/test_rule_registry.py @@ -1,7 +1,8 @@ """Unit tests for the rule registry.""" import pytest -from dbt_score.config_parser import DbtScoreConfig +from dbt_score import Severity +from dbt_score.config import Config from dbt_score.exceptions import DuplicatedRuleException from dbt_score.rule_registry import RuleRegistry @@ -18,7 +19,7 @@ def test_rule_registry_discovery(): def test_disabled_rule_registry_discovery(): """Ensure disabled rules are not discovered.""" - config = DbtScoreConfig() + config = Config() config.disabled_rules = ["tests.rules.nested.example.rule_test_nested_example"] r = RuleRegistry(config) r._load("tests.rules") @@ -27,6 +28,18 @@ def test_disabled_rule_registry_discovery(): ] +def test_configured_rule_registry_discovery(valid_config_path): + """Ensure rules are discovered and configured correctly.""" + config = Config() + config.load_toml_file(str(valid_config_path)) + r = RuleRegistry(config) + r._load("tests.rules") + r.init_rules() + assert ( + r.rules["tests.rules.example.rule_test_example"].severity == Severity.CRITICAL + ) + + def test_rule_registry_no_duplicates(): """Ensure no duplicate rule names can coexist.""" r = RuleRegistry() From edc27668f7fa5694d466c0b31e5118f086e26317 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:35:57 +0200 Subject: [PATCH 14/29] Make config defaults Final --- src/dbt_score/config.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index 8f03733..f002a5e 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -2,7 +2,7 @@ import logging from dataclasses import dataclass, field -from typing import Any, ClassVar +from typing import Any, Final import tomllib @@ -29,9 +29,9 @@ def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": class Config: """Configuration for dbt-score.""" - _main_section = "tool.dbt-score" - _options: ClassVar[list[str]] = ["rule_namespaces", "disabled_rules"] - _rules_section = f"{_main_section}.rules" + _main_section: Final = "tool.dbt-score" + _options: Final = ["rule_namespaces", "disabled_rules"] + _rules_section: Final = f"{_main_section}.rules" def __init__(self) -> None: """Initialize the Config object.""" From 566b56162c8ff0cdbdd50b303e07601f0852da31 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:38:35 +0200 Subject: [PATCH 15/29] Drop python 3.10 support --- .github/workflows/ci.yml | 4 ++-- pyproject.toml | 2 +- src/dbt_score/config.py | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed41416..6cd6760 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,14 +18,14 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python: ["3.10", "3.11"] + python: ["3.11"] steps: - uses: actions/checkout@v4 - name: Set up PDM uses: pdm-project/setup-pdm@v4 with: - python-version: "3.10" + python-version: "3.11" - name: Install dependencies run: | pdm sync -d diff --git a/pyproject.toml b/pyproject.toml index 9aae426..aa0e99b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ dependencies = [ "dbt-core>=1.5", "click>=7.1.1, <9.0.0", ] -requires-python = ">=3.10" +requires-python = ">=3.11" readme = "README.md" license = {text = "MIT"} diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index f002a5e..853ebb1 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -1,11 +1,10 @@ """This module is responsible for loading configuration.""" import logging +import tomllib from dataclasses import dataclass, field from typing import Any, Final -import tomllib - logger = logging.getLogger(__name__) DEFAULT_CONFIG_FILE = "pyproject.toml" From 1abff4f9cfa42777ee8e2910d7f73da38fc1f4e7 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:46:41 +0200 Subject: [PATCH 16/29] Rename config_parser to config --- docs/reference/config.md | 3 +++ docs/reference/config_parser.md | 3 --- mkdocs.yml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) create mode 100644 docs/reference/config.md delete mode 100644 docs/reference/config_parser.md diff --git a/docs/reference/config.md b/docs/reference/config.md new file mode 100644 index 0000000..b3687e4 --- /dev/null +++ b/docs/reference/config.md @@ -0,0 +1,3 @@ +# Config + +::: dbt_score.config diff --git a/docs/reference/config_parser.md b/docs/reference/config_parser.md deleted file mode 100644 index 55d52ad..0000000 --- a/docs/reference/config_parser.md +++ /dev/null @@ -1,3 +0,0 @@ -# Config parser - -::: dbt_score.config_parser diff --git a/mkdocs.yml b/mkdocs.yml index cb34ef4..16c887d 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -8,7 +8,7 @@ nav: - Home: index.md - Reference: - reference/cli.md - - reference/config_parser.md + - reference/config.md - reference/exceptions.md - reference/evaluation.md - reference/models.md From 7abc430b2917910d83abb62371c529022ce39589 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:49:20 +0200 Subject: [PATCH 17/29] Revert copying rule config --- src/dbt_score/config.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index 853ebb1..bbf4325 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -20,9 +20,8 @@ class RuleConfig: @staticmethod def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": """Create a RuleConfig from a dictionary.""" - copy = rule_config.copy() - severity = copy.pop("severity", None) - return RuleConfig(severity=severity, params=copy) + severity = rule_config.pop("severity", None) + return RuleConfig(severity=severity, params=rule_config) class Config: From 8cadd1af0ff7a86a08b550fadec2c136617f1454 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 11:58:39 +0200 Subject: [PATCH 18/29] Improve logging and error --- src/dbt_score/config.py | 3 ++- src/dbt_score/rule.py | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index bbf4325..88a59bf 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -47,6 +47,7 @@ def load_toml_file(self, file: str) -> None: toml_data = tomllib.load(f) tools = toml_data.get("tool", {}) dbt_score_config = tools.get("dbt-score", {}) + rules_config = dbt_score_config.pop("rules", {}) # Main configuration for option, value in dbt_score_config.items(): @@ -61,5 +62,5 @@ def load_toml_file(self, file: str) -> None: # Rule configuration self.rules_config = { name: RuleConfig.from_dict(config) - for name, config in dbt_score_config.get("rules", {}).items() + for name, config in rules_config.items() } diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 60ad5f2..a17439a 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -55,7 +55,9 @@ def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: if k in self.default_params: rule_params[k] = v else: - raise AttributeError(f"Unknown rule parameter: {k}.") + raise AttributeError( + f"Unknown rule parameter: {k} for rule {self.source()}." + ) self.set_severity( rule_config.severity From 862da7f044fdbf7eb23bcc860a1b7912c391fcd1 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 12:00:51 +0200 Subject: [PATCH 19/29] Remove unused fixtures --- tests/conftest.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 376f8ca..ca096e0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,7 +5,6 @@ from typing import Any, Type from dbt_score import Model, Rule, RuleViolation, Severity, rule -from dbt_score.config import Config, RuleConfig from pytest import fixture # Configuration @@ -23,18 +22,6 @@ def invalid_config_path() -> Path: return Path(__file__).parent / "resources" / "invalid_pyproject.toml" -@fixture -def default_config() -> Config: - """Return a Config object.""" - return Config() - - -@fixture -def default_rule_config() -> RuleConfig: - """Return an empty RuleConfig object.""" - return RuleConfig() - - # Manifest From 06e0957d9882919ac94cb7060b2867c1bdd8120e Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 12:06:42 +0200 Subject: [PATCH 20/29] Improve test --- tests/test_config.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_config.py b/tests/test_config.py index 4afda8b..0562571 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -25,7 +25,11 @@ def test_load_invalid_toml_file(caplog, invalid_config_path): def test_invalid_rule_config(rule_severity_low): """Test that an invalid rule config raises an exception.""" config = RuleConfig(params={"foo": "bar"}) - with pytest.raises(AttributeError, match="Unknown rule parameter: foo."): + with pytest.raises( + AttributeError, + match="Unknown rule parameter: foo for rule " + "tests.conftest.rule_severity_low.", + ): rule_severity_low(config) From da190b7a6a6d57df042b39aa1a185fc6592fe409 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 13:38:23 +0200 Subject: [PATCH 21/29] Fix python version in readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d3c92a4..e7ecde0 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ Linter for dbt model metadata. You'll need the following prerequisites: -- Any Python version starting from 3.10 +- Any Python version starting from 3.11 - [pre-commit](https://pre-commit.com/) - [PDM](https://pdm-project.org/2.12/) From 12d49fc1c5387114558606d2e2cb1643990cffc3 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 13:59:17 +0200 Subject: [PATCH 22/29] Fix type hints --- src/dbt_score/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index 88a59bf..8fa2d64 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -27,9 +27,9 @@ def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": class Config: """Configuration for dbt-score.""" - _main_section: Final = "tool.dbt-score" - _options: Final = ["rule_namespaces", "disabled_rules"] - _rules_section: Final = f"{_main_section}.rules" + _main_section: Final[str] = "tool.dbt-score" + _options: Final[tuple[str, ...]] = ("rule_namespaces", "disabled_rules") + _rules_section: Final[str] = f"{_main_section}.rules" def __init__(self) -> None: """Initialize the Config object.""" From ae091fe1225a41a240044d8ff3351e432e9ebee1 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 3 May 2024 15:23:37 +0200 Subject: [PATCH 23/29] Address comments --- .github/workflows/ci.yml | 2 +- pdm.lock | 29 +---------------------------- src/dbt_score/cli.py | 5 ++--- src/dbt_score/config.py | 29 ++++++++++++++++++++++++----- src/dbt_score/rule.py | 9 +++++---- tests/test_cli.py | 8 ++++---- tests/test_config.py | 21 +++++++++++++++++++-- tests/test_evaluation.py | 2 +- tests/test_rule_registry.py | 2 +- 9 files changed, 58 insertions(+), 49 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6cd6760..e8f7941 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,7 +18,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python: ["3.11"] + python: ["3.11", "3.12"] steps: - uses: actions/checkout@v4 diff --git a/pdm.lock b/pdm.lock index ecd8b52..f1de9f0 100644 --- a/pdm.lock +++ b/pdm.lock @@ -5,7 +5,7 @@ groups = ["default", "dev", "docs", "lint", "test"] strategy = ["cross_platform"] lock_version = "4.4.1" -content_hash = "sha256:41d1ad10106c411809e42ab06b1bed7dabdf42afba377f62c45df6189fe01986" +content_hash = "sha256:c3a671a78ccbbea1806a039799632f5f7d668036bef35756fd42240dc9dbfbea" [[package]] name = "agate" @@ -288,7 +288,6 @@ requires_python = ">=3.8" summary = "Code coverage measurement for Python" dependencies = [ "coverage==7.4.3", - "tomli; python_full_version <= \"3.11.0a6\"", ] files = [ {file = "coverage-7.4.3-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:8580b827d4746d47294c0e0b92854c85a92c2227927433998f0d3320ae8a71b6"}, @@ -414,16 +413,6 @@ files = [ {file = "distlib-0.3.8.tar.gz", hash = "sha256:1530ea13e350031b6312d8580ddb6b27a104275a31106523b8f123787f494f64"}, ] -[[package]] -name = "exceptiongroup" -version = "1.2.0" -requires_python = ">=3.7" -summary = "Backport of PEP 654 (exception groups)" -files = [ - {file = "exceptiongroup-1.2.0-py3-none-any.whl", hash = "sha256:4bfd3996ac73b41e9b9628b04e079f193850720ea5945fc96a08633c66912f14"}, - {file = "exceptiongroup-1.2.0.tar.gz", hash = "sha256:91f5c769735f051a4290d52edd0858999b57e5876e9f85937691bd4c9fa3ed68"}, -] - [[package]] name = "filelock" version = "3.13.1" @@ -874,7 +863,6 @@ requires_python = ">=3.8" summary = "Optional static typing for Python" dependencies = [ "mypy-extensions>=1.0.0", - "tomli>=1.1.0; python_version < \"3.11\"", "typing-extensions>=4.1.0", ] files = [ @@ -1139,7 +1127,6 @@ requires_python = ">=3.8" summary = "API to interact with the python pyproject.toml based projects" dependencies = [ "packaging>=23.1", - "tomli>=2.0.1; python_version < \"3.11\"", ] files = [ {file = "pyproject_api-1.6.1-py3-none-any.whl", hash = "sha256:4c0116d60476b0786c88692cf4e325a9814965e2469c5998b830bba16b183675"}, @@ -1153,11 +1140,9 @@ requires_python = ">=3.8" summary = "pytest: simple powerful testing with Python" dependencies = [ "colorama; sys_platform == \"win32\"", - "exceptiongroup>=1.0.0rc8; python_version < \"3.11\"", "iniconfig", "packaging", "pluggy<2.0,>=1.4", - "tomli>=1; python_version < \"3.11\"", ] files = [ {file = "pytest-8.1.0-py3-none-any.whl", hash = "sha256:ee32db7af8de4629a455806befa90559f307424c07b8413ccfc30bf5b221dd7e"}, @@ -1523,16 +1508,6 @@ files = [ {file = "text_unidecode-1.3-py2.py3-none-any.whl", hash = "sha256:1311f10e8b895935241623731c2ba64f4c455287888b18189350b67134a822e8"}, ] -[[package]] -name = "tomli" -version = "2.0.1" -requires_python = ">=3.7" -summary = "A lil' TOML parser" -files = [ - {file = "tomli-2.0.1-py3-none-any.whl", hash = "sha256:939de3e7a6161af0c887ef91b7d41a53e7c5a1ca976325f429cb46ea9bc30ecc"}, - {file = "tomli-2.0.1.tar.gz", hash = "sha256:de526c12914f0c550d15924c62d72abc48d6fe7364aa87328337a31007fe8a4f"}, -] - [[package]] name = "tox" version = "4.13.0" @@ -1547,7 +1522,6 @@ dependencies = [ "platformdirs>=4.1", "pluggy>=1.3", "pyproject-api>=1.6.1", - "tomli>=2.0.1; python_version < \"3.11\"", "virtualenv>=20.25", ] files = [ @@ -1561,7 +1535,6 @@ version = "0.7.2" requires_python = ">=3.7" summary = "A plugin for tox that utilizes PDM as the package manager and installer" dependencies = [ - "tomli; python_version < \"3.11\"", "tox>=4.0", ] files = [ diff --git a/src/dbt_score/cli.py b/src/dbt_score/cli.py index b87537a..e629996 100644 --- a/src/dbt_score/cli.py +++ b/src/dbt_score/cli.py @@ -7,7 +7,7 @@ from click.core import ParameterSource from dbt.cli.options import MultiOption -from dbt_score.config import DEFAULT_CONFIG_FILE, Config +from dbt_score.config import Config from dbt_score.lint import lint_dbt_project from dbt_score.parse import dbt_parse, get_default_manifest_path @@ -59,8 +59,7 @@ def lint(select: tuple[str], manifest: Path, run_dbt_parse: bool) -> None: raise click.UsageError("--run-dbt-parse cannot be used with --manifest.") config = Config() - if Path(DEFAULT_CONFIG_FILE).exists(): - config.load_toml_file(DEFAULT_CONFIG_FILE) + config.load() if run_dbt_parse: dbt_parse() diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index 8fa2d64..d2798c5 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -3,6 +3,7 @@ import logging import tomllib from dataclasses import dataclass, field +from pathlib import Path from typing import Any, Final logger = logging.getLogger(__name__) @@ -28,7 +29,7 @@ class Config: """Configuration for dbt-score.""" _main_section: Final[str] = "tool.dbt-score" - _options: Final[tuple[str, ...]] = ("rule_namespaces", "disabled_rules") + _options: Final[list[str]] = ["rule_namespaces", "disabled_rules"] _rules_section: Final[str] = f"{_main_section}.rules" def __init__(self) -> None: @@ -36,12 +37,13 @@ def __init__(self) -> None: self.rule_namespaces: list[str] = ["dbt_score_rules"] self.disabled_rules: list[str] = [] self.rules_config: dict[str, RuleConfig] = {} + self.config_file: Path | None = None def set_option(self, option: str, value: Any) -> None: """Set an option in the config.""" setattr(self, option, value) - def load_toml_file(self, file: str) -> None: + def _load_toml_file(self, file: str) -> None: """Load the options from a TOML file.""" with open(file, "rb") as f: toml_data = tomllib.load(f) @@ -51,10 +53,11 @@ def load_toml_file(self, file: str) -> None: # Main configuration for option, value in dbt_score_config.items(): - # If value is a dictionary, it's another section - if option in self._options and not isinstance(value, dict): + if option in self._options: self.set_option(option, value) - else: + elif not isinstance( + value, dict + ): # If value is a dictionary, it's another section logger.warning( f"Option {option} in {self._main_section} not supported." ) @@ -64,3 +67,19 @@ def load_toml_file(self, file: str) -> None: name: RuleConfig.from_dict(config) for name, config in rules_config.items() } + + def get_config_file(self, directory: Path) -> None: + """Get the config file.""" + candidates = [directory] + candidates.extend(directory.parents) + for path in candidates: + config_file = path / DEFAULT_CONFIG_FILE + if config_file.exists(): + self.config_file = config_file + break + + def load(self) -> None: + """Load the config.""" + self.get_config_file(Path.cwd()) + if self.config_file: + self._load_toml_file(str(self.config_file)) diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index a17439a..72b701e 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -38,7 +38,9 @@ class Rule: def __init__(self, rule_config: RuleConfig | None = None) -> None: """Initialize the rule.""" - self.params = self.process_config(rule_config) if rule_config else {} + self.params: dict[str, Any] = {} + if rule_config: + self.process_config(rule_config) def __init_subclass__(cls, **kwargs) -> None: # type: ignore """Initializes the subclass.""" @@ -46,7 +48,7 @@ def __init_subclass__(cls, **kwargs) -> None: # type: ignore if not hasattr(cls, "description"): raise AttributeError("Subclass must define class attribute `description`.") - def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: + def process_config(self, rule_config: RuleConfig) -> None: """Process the rule config.""" rule_params = self.default_params.copy() @@ -62,8 +64,7 @@ def process_config(self, rule_config: RuleConfig) -> dict[str, Any]: self.set_severity( rule_config.severity ) if rule_config.severity else rule_config.severity - - return rule_params + self.params = rule_params def evaluate(self, model: Model) -> RuleViolation | None: """Evaluates the rule.""" diff --git a/tests/test_cli.py b/tests/test_cli.py index e4cdd78..faae3b7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9,7 +9,7 @@ def test_invalid_options(): """Test invalid cli options.""" runner = CliRunner() - with patch("dbt_score.cli.Config.load_toml_file"): + with patch("dbt_score.cli.Config._load_toml_file"): result = runner.invoke( lint, ["--manifest", "fake_manifest.json", "--run-dbt-parse"] ) @@ -18,7 +18,7 @@ def test_invalid_options(): def test_lint_existing_manifest(manifest_path): """Test lint with an existing manifest.""" - with patch("dbt_score.cli.Config.load_toml_file"): + with patch("dbt_score.cli.Config._load_toml_file"): runner = CliRunner() result = runner.invoke(lint, ["--manifest", manifest_path]) assert result.exit_code == 0 @@ -30,12 +30,12 @@ def test_lint_non_existing_manifest(): # Provide manifest in command line with pytest.raises(FileNotFoundError): - with patch("dbt_score.cli.Config.load_toml_file"): + with patch("dbt_score.cli.Config._load_toml_file"): runner.invoke( lint, ["--manifest", "fake_manifest.json"], catch_exceptions=False ) # Use default manifest path with pytest.raises(FileNotFoundError): - with patch("dbt_score.cli.Config.load_toml_file"): + with patch("dbt_score.cli.Config._load_toml_file"): runner.invoke(lint, catch_exceptions=False) diff --git a/tests/test_config.py b/tests/test_config.py index 0562571..5fdc6f0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,4 +1,5 @@ """Tests for the module config_parser.""" +from pathlib import Path import pytest from dbt_score.config import Config, RuleConfig @@ -8,7 +9,7 @@ def test_load_valid_toml_file(valid_config_path): """Test that a valid config file loads correctly.""" config = Config() - config.load_toml_file(str(valid_config_path)) + config._load_toml_file(str(valid_config_path)) assert config.rule_namespaces == ["namespace_foo"] assert config.disabled_rules == ["foo", "bar"] assert config.rules_config["foobar"].severity == 4 @@ -18,7 +19,7 @@ def test_load_valid_toml_file(valid_config_path): def test_load_invalid_toml_file(caplog, invalid_config_path): """Test that an invalid config file logs a warning.""" config = Config() - config.load_toml_file(str(invalid_config_path)) + config._load_toml_file(str(invalid_config_path)) assert "Option foo in tool.dbt-score not supported." in caplog.text @@ -40,3 +41,19 @@ def test_valid_rule_config(valid_config_path, rule_with_params): assert rule_with_params.severity == Severity.CRITICAL assert rule_with_params.default_params == {"model_name": "model1"} assert rule_with_params.params == {"model_name": "baz"} + + +def test_get_config_file(): + """Test that the config file is found in the current directory.""" + directory = Path(__file__).parent / "resources" + config = Config() + config.get_config_file(directory) + assert config.config_file == directory / "pyproject.toml" + + +def test_get_parent_config_file(): + """Test that the config file is found in the parent directory.""" + directory = Path(__file__).parent / "resources" / "sub_dir" + config = Config() + config.get_config_file(directory) + assert config.config_file == directory.parent / "pyproject.toml" diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index d3ac1df..69227d9 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -146,7 +146,7 @@ def test_evaluation_rule_with_params( model2 = manifest_loader.models[1] config = Config() - config.load_toml_file(str(valid_config_path)) + config._load_toml_file(str(valid_config_path)) rule_registry = RuleRegistry(config) rule_registry._add_rule("rule_with_params", rule_with_params) diff --git a/tests/test_rule_registry.py b/tests/test_rule_registry.py index ac7a98d..128175a 100644 --- a/tests/test_rule_registry.py +++ b/tests/test_rule_registry.py @@ -31,7 +31,7 @@ def test_disabled_rule_registry_discovery(): def test_configured_rule_registry_discovery(valid_config_path): """Ensure rules are discovered and configured correctly.""" config = Config() - config.load_toml_file(str(valid_config_path)) + config._load_toml_file(str(valid_config_path)) r = RuleRegistry(config) r._load("tests.rules") r.init_rules() From 99c4da37d2ad866fe08d51c44e14d941fa73c211 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 10 May 2024 08:58:01 +0200 Subject: [PATCH 24/29] Update tests/test_cli.py Co-authored-by: Kirill Druzhinin <112948926+druzhinin-kirill@users.noreply.github.com> --- tests/test_cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_cli.py b/tests/test_cli.py index faae3b7..f22f7c7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,4 +1,5 @@ """Test the CLI.""" + from unittest.mock import patch import pytest From a50d027391958b37205296c1350562957b0c42c1 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 10 May 2024 09:06:16 +0200 Subject: [PATCH 25/29] Require config in RuleRegistry --- src/dbt_score/rule_registry.py | 4 ++-- tests/conftest.py | 7 +++++++ tests/test_evaluation.py | 21 ++++++++++----------- tests/test_rule_registry.py | 12 ++++++------ 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index 44a754f..faf2c49 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -18,9 +18,9 @@ class RuleRegistry: """A container for configured rules.""" - def __init__(self, config: Config | None = None) -> None: + def __init__(self, config: Config) -> None: """Instantiate a rule registry.""" - self.config = config or Config() + self.config = config self._rules: dict[str, Type[Rule]] = {} self._initialized_rules: dict[str, Rule] = {} diff --git a/tests/conftest.py b/tests/conftest.py index ca096e0..b5d1d98 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,11 +5,18 @@ from typing import Any, Type from dbt_score import Model, Rule, RuleViolation, Severity, rule +from dbt_score.config import Config from pytest import fixture # Configuration +@fixture() +def default_config() -> Config: + """Return a default Config object.""" + return Config() + + @fixture def valid_config_path() -> Path: """Return the path of the configuration.""" diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index 69227d9..25b85ef 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -15,13 +15,14 @@ def test_evaluation_low_medium_high( rule_severity_medium, rule_severity_high, rule_error, + default_config, ): """Test rule evaluation with a combination of LOW, MEDIUM and HIGH severity.""" manifest_loader = ManifestLoader(manifest_path) mock_formatter = Mock() mock_scorer = Mock() - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(default_config) rule_registry._add_rule("rule_severity_low", rule_severity_low) rule_registry._add_rule("rule_severity_medium", rule_severity_medium) rule_registry._add_rule("rule_severity_high", rule_severity_high) @@ -57,14 +58,12 @@ def test_evaluation_low_medium_high( def test_evaluation_critical( - manifest_path, - rule_severity_low, - rule_severity_critical, + manifest_path, rule_severity_low, rule_severity_critical, default_config ): """Test rule evaluation with a CRITICAL severity.""" manifest_loader = ManifestLoader(manifest_path) - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(default_config) rule_registry._add_rule("rule_severity_low", rule_severity_low) rule_registry._add_rule("rule_severity_critical", rule_severity_critical) rule_registry.init_rules() @@ -82,11 +81,11 @@ def test_evaluation_critical( assert isinstance(evaluation.results[model2][rule_severity_critical], RuleViolation) -def test_evaluation_no_rule(manifest_path): +def test_evaluation_no_rule(manifest_path, default_config): """Test rule evaluation when no rule exists.""" manifest_loader = ManifestLoader(manifest_path) - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(default_config) evaluation = Evaluation( rule_registry=rule_registry, @@ -100,11 +99,11 @@ def test_evaluation_no_rule(manifest_path): assert len(results) == 0 -def test_evaluation_no_model(manifest_empty_path, rule_severity_low): +def test_evaluation_no_model(manifest_empty_path, rule_severity_low, default_config): """Test rule evaluation when no model exists.""" manifest_loader = ManifestLoader(manifest_empty_path) - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(default_config) rule_registry._add_rule("rule_severity_low", rule_severity_low) evaluation = Evaluation( @@ -119,11 +118,11 @@ def test_evaluation_no_model(manifest_empty_path, rule_severity_low): assert list(evaluation.scores.values()) == [] -def test_evaluation_no_model_no_rule(manifest_empty_path): +def test_evaluation_no_model_no_rule(manifest_empty_path, default_config): """Test rule evaluation when no rule and no model exists.""" manifest_loader = ManifestLoader(manifest_empty_path) - rule_registry = RuleRegistry() + rule_registry = RuleRegistry(default_config) evaluation = Evaluation( rule_registry=rule_registry, diff --git a/tests/test_rule_registry.py b/tests/test_rule_registry.py index 128175a..3049dad 100644 --- a/tests/test_rule_registry.py +++ b/tests/test_rule_registry.py @@ -7,9 +7,9 @@ from dbt_score.rule_registry import RuleRegistry -def test_rule_registry_discovery(): +def test_rule_registry_discovery(default_config): """Ensure rules can be found in a given namespace recursively.""" - r = RuleRegistry() + r = RuleRegistry(default_config) r._load("tests.rules") assert sorted(r._rules.keys()) == [ "tests.rules.example.rule_test_example", @@ -40,16 +40,16 @@ def test_configured_rule_registry_discovery(valid_config_path): ) -def test_rule_registry_no_duplicates(): +def test_rule_registry_no_duplicates(default_config): """Ensure no duplicate rule names can coexist.""" - r = RuleRegistry() + r = RuleRegistry(default_config) r._load("tests.rules") with pytest.raises(DuplicatedRuleException): r._load("tests.rules") -def test_rule_registry_core_rules(): +def test_rule_registry_core_rules(default_config): """Ensure core rules are automatically discovered.""" - r = RuleRegistry() + r = RuleRegistry(default_config) r.load_all() assert len(r.rules) > 0 From 52080c06a05004f8e462696effebe2a7ee126db6 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Fri, 10 May 2024 09:33:55 +0200 Subject: [PATCH 26/29] Convert Severity in RuleConfig --- src/dbt_score/config.py | 55 +++++++++++++--------------------- src/dbt_score/rule.py | 27 +++++++++++++---- src/dbt_score/rule_registry.py | 4 +-- tests/test_config.py | 13 ++++---- 4 files changed, 53 insertions(+), 46 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index d2798c5..b2d09d0 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -2,29 +2,16 @@ import logging import tomllib -from dataclasses import dataclass, field from pathlib import Path from typing import Any, Final +from dbt_score.rule import RuleConfig + logger = logging.getLogger(__name__) DEFAULT_CONFIG_FILE = "pyproject.toml" -@dataclass -class RuleConfig: - """Configuration for a rule.""" - - severity: int | None = None - params: dict[str, Any] = field(default_factory=dict) - - @staticmethod - def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": - """Create a RuleConfig from a dictionary.""" - severity = rule_config.pop("severity", None) - return RuleConfig(severity=severity, params=rule_config) - - class Config: """Configuration for dbt-score.""" @@ -47,26 +34,26 @@ def _load_toml_file(self, file: str) -> None: """Load the options from a TOML file.""" with open(file, "rb") as f: toml_data = tomllib.load(f) - tools = toml_data.get("tool", {}) - dbt_score_config = tools.get("dbt-score", {}) - rules_config = dbt_score_config.pop("rules", {}) - - # Main configuration - for option, value in dbt_score_config.items(): - if option in self._options: - self.set_option(option, value) - elif not isinstance( - value, dict - ): # If value is a dictionary, it's another section - logger.warning( - f"Option {option} in {self._main_section} not supported." - ) - # Rule configuration - self.rules_config = { - name: RuleConfig.from_dict(config) - for name, config in rules_config.items() - } + tools = toml_data.get("tool", {}) + dbt_score_config = tools.get("dbt-score", {}) + rules_config = dbt_score_config.pop("rules", {}) + + # Main configuration + for option, value in dbt_score_config.items(): + if option in self._options: + self.set_option(option, value) + elif not isinstance( + value, dict + ): # If value is a dictionary, it's another section + logger.warning( + f"Option {option} in {self._main_section} not supported." + ) + + # Rule configuration + self.rules_config = { + name: RuleConfig.from_dict(config) for name, config in rules_config.items() + } def get_config_file(self, directory: Path) -> None: """Get the config file.""" diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 72b701e..6cc716a 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -2,11 +2,10 @@ import inspect import typing -from dataclasses import dataclass +from dataclasses import dataclass, field from enum import Enum from typing import Any, Callable, Type, TypeAlias, overload -from dbt_score.config import RuleConfig from dbt_score.models import Model @@ -19,6 +18,26 @@ class Severity(Enum): CRITICAL = 4 +@dataclass +class RuleConfig: + """Configuration for a rule.""" + + severity: Severity | None = None + params: dict[str, Any] = field(default_factory=dict) + + @staticmethod + def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": + """Create a RuleConfig from a dictionary.""" + rule_config = rule_config.copy() + severity = ( + Severity(rule_config.pop("severity", None)) + if "severity" in rule_config + else None + ) + + return RuleConfig(severity=severity, params=rule_config) + + @dataclass class RuleViolation: """The violation of a rule.""" @@ -71,10 +90,8 @@ def evaluate(self, model: Model) -> RuleViolation | None: raise NotImplementedError("Subclass must implement method `evaluate`.") @classmethod - def set_severity(cls, severity: int | Severity) -> None: + def set_severity(cls, severity: Severity) -> None: """Set the severity of the rule.""" - if isinstance(severity, int): - severity = Severity(severity) cls.severity = severity @classmethod diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index faf2c49..bc0c567 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -8,9 +8,9 @@ import pkgutil from typing import Iterator, Type -from dbt_score.config import Config, RuleConfig +from dbt_score.config import Config from dbt_score.exceptions import DuplicatedRuleException -from dbt_score.rule import Rule +from dbt_score.rule import Rule, RuleConfig logger = logging.getLogger(__name__) diff --git a/tests/test_config.py b/tests/test_config.py index 5fdc6f0..fdb3389 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -2,8 +2,8 @@ from pathlib import Path import pytest -from dbt_score.config import Config, RuleConfig -from dbt_score.rule import Severity +from dbt_score.config import Config +from dbt_score.rule import RuleConfig, Severity def test_load_valid_toml_file(valid_config_path): @@ -12,8 +12,11 @@ def test_load_valid_toml_file(valid_config_path): config._load_toml_file(str(valid_config_path)) assert config.rule_namespaces == ["namespace_foo"] assert config.disabled_rules == ["foo", "bar"] - assert config.rules_config["foobar"].severity == 4 - assert config.rules_config["tests.rules.example.rule_test_example"].severity == 4 + assert config.rules_config["foobar"].severity == Severity.CRITICAL + assert ( + config.rules_config["tests.rules.example.rule_test_example"].severity + == Severity.CRITICAL + ) def test_load_invalid_toml_file(caplog, invalid_config_path): @@ -36,7 +39,7 @@ def test_invalid_rule_config(rule_severity_low): def test_valid_rule_config(valid_config_path, rule_with_params): """Test that a valid rule config can be loaded.""" - config = RuleConfig(severity=4, params={"model_name": "baz"}) + config = RuleConfig(severity=Severity(4), params={"model_name": "baz"}) rule_with_params = rule_with_params(config) assert rule_with_params.severity == Severity.CRITICAL assert rule_with_params.default_params == {"model_name": "model1"} From 923d0204fdc3b1f355d08ce2b10036979507e912 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Mon, 13 May 2024 10:25:38 +0200 Subject: [PATCH 27/29] Change params to config and make get_config_file static --- src/dbt_score/config.py | 12 ++++++------ src/dbt_score/evaluation.py | 2 +- src/dbt_score/rule.py | 28 ++++++++++++++-------------- tests/conftest.py | 10 +++++----- tests/resources/pyproject.toml | 2 +- tests/test_config.py | 22 +++++++++++----------- tests/test_evaluation.py | 14 +++++++------- 7 files changed, 45 insertions(+), 45 deletions(-) diff --git a/src/dbt_score/config.py b/src/dbt_score/config.py index b2d09d0..aaa360c 100644 --- a/src/dbt_score/config.py +++ b/src/dbt_score/config.py @@ -55,18 +55,18 @@ def _load_toml_file(self, file: str) -> None: name: RuleConfig.from_dict(config) for name, config in rules_config.items() } - def get_config_file(self, directory: Path) -> None: + @staticmethod + def get_config_file(directory: Path) -> Path | None: """Get the config file.""" candidates = [directory] candidates.extend(directory.parents) for path in candidates: config_file = path / DEFAULT_CONFIG_FILE if config_file.exists(): - self.config_file = config_file - break + return config_file def load(self) -> None: """Load the config.""" - self.get_config_file(Path.cwd()) - if self.config_file: - self._load_toml_file(str(self.config_file)) + config_file = self.get_config_file(Path.cwd()) + if config_file: + self._load_toml_file(str(config_file)) diff --git a/src/dbt_score/evaluation.py b/src/dbt_score/evaluation.py index 4790395..284f1db 100644 --- a/src/dbt_score/evaluation.py +++ b/src/dbt_score/evaluation.py @@ -57,7 +57,7 @@ def evaluate(self) -> None: self.results[model] = {} for rule in rules: try: - result: RuleViolation | None = rule.evaluate(model, **rule.params) + result: RuleViolation | None = rule.evaluate(model, **rule.config) except Exception as e: self.results[model][rule.__class__] = e else: diff --git a/src/dbt_score/rule.py b/src/dbt_score/rule.py index 6cc716a..541c06a 100644 --- a/src/dbt_score/rule.py +++ b/src/dbt_score/rule.py @@ -23,19 +23,19 @@ class RuleConfig: """Configuration for a rule.""" severity: Severity | None = None - params: dict[str, Any] = field(default_factory=dict) + config: dict[str, Any] = field(default_factory=dict) @staticmethod def from_dict(rule_config: dict[str, Any]) -> "RuleConfig": """Create a RuleConfig from a dictionary.""" - rule_config = rule_config.copy() + config = rule_config.copy() severity = ( - Severity(rule_config.pop("severity", None)) + Severity(config.pop("severity", None)) if "severity" in rule_config else None ) - return RuleConfig(severity=severity, params=rule_config) + return RuleConfig(severity=severity, config=config) @dataclass @@ -53,11 +53,11 @@ class Rule: description: str severity: Severity = Severity.MEDIUM - default_params: typing.ClassVar[dict[str, Any]] = {} + default_config: typing.ClassVar[dict[str, Any]] = {} def __init__(self, rule_config: RuleConfig | None = None) -> None: """Initialize the rule.""" - self.params: dict[str, Any] = {} + self.config: dict[str, Any] = {} if rule_config: self.process_config(rule_config) @@ -69,12 +69,12 @@ def __init_subclass__(cls, **kwargs) -> None: # type: ignore def process_config(self, rule_config: RuleConfig) -> None: """Process the rule config.""" - rule_params = self.default_params.copy() + config = self.default_config.copy() - # Overwrite default rule params - for k, v in rule_config.params.items(): - if k in self.default_params: - rule_params[k] = v + # Overwrite default rule configuration + for k, v in rule_config.config.items(): + if k in self.default_config: + config[k] = v else: raise AttributeError( f"Unknown rule parameter: {k} for rule {self.source()}." @@ -83,7 +83,7 @@ def process_config(self, rule_config: RuleConfig) -> None: self.set_severity( rule_config.severity ) if rule_config.severity else rule_config.severity - self.params = rule_params + self.config = config def evaluate(self, model: Model) -> RuleViolation | None: """Evaluates the rule.""" @@ -159,7 +159,7 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: return func(*args, **kwargs) # Get default parameters from the rule definition - default_params = { + default_config = { key: val.default for key, val in inspect.signature(func).parameters.items() if val.default != inspect.Parameter.empty @@ -172,7 +172,7 @@ def wrapped_func(self: Rule, *args: Any, **kwargs: Any) -> RuleViolation | None: { "description": rule_description, "severity": severity, - "default_params": default_params, + "default_config": default_config, "evaluate": wrapped_func, # Forward origin of the decorated function "__qualname__": func.__qualname__, # https://peps.python.org/pep-3155/ diff --git a/tests/conftest.py b/tests/conftest.py index b5d1d98..980cb14 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -179,18 +179,18 @@ def rule_severity_critical(model: Model) -> RuleViolation | None: @fixture -def rule_with_params() -> Type[Rule]: - """An example rule with additional input params.""" +def rule_with_config() -> Type[Rule]: + """An example rule with additional configuration.""" @rule - def rule_with_params( + def rule_with_config( model: Model, model_name: str = "model1" ) -> RuleViolation | None: - """Rule with additional input params.""" + """Rule with additional configuration.""" if model.name != model_name: return RuleViolation(message=model_name) - return rule_with_params + return rule_with_config @fixture diff --git a/tests/resources/pyproject.toml b/tests/resources/pyproject.toml index e496972..8db8981 100644 --- a/tests/resources/pyproject.toml +++ b/tests/resources/pyproject.toml @@ -6,7 +6,7 @@ disabled_rules = ["foo", "bar"] [tool.dbt-score.rules.foobar] severity=4 -[tool.dbt-score.rules.rule_with_params] +[tool.dbt-score.rules.rule_with_config] model_name="model2" [tool.dbt-score.rules."tests.rules.example.rule_test_example"] diff --git a/tests/test_config.py b/tests/test_config.py index fdb3389..24a9ae0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -28,7 +28,7 @@ def test_load_invalid_toml_file(caplog, invalid_config_path): def test_invalid_rule_config(rule_severity_low): """Test that an invalid rule config raises an exception.""" - config = RuleConfig(params={"foo": "bar"}) + config = RuleConfig(config={"foo": "bar"}) with pytest.raises( AttributeError, match="Unknown rule parameter: foo for rule " @@ -37,26 +37,26 @@ def test_invalid_rule_config(rule_severity_low): rule_severity_low(config) -def test_valid_rule_config(valid_config_path, rule_with_params): +def test_valid_rule_config(valid_config_path, rule_with_config): """Test that a valid rule config can be loaded.""" - config = RuleConfig(severity=Severity(4), params={"model_name": "baz"}) - rule_with_params = rule_with_params(config) - assert rule_with_params.severity == Severity.CRITICAL - assert rule_with_params.default_params == {"model_name": "model1"} - assert rule_with_params.params == {"model_name": "baz"} + config = RuleConfig(severity=Severity(4), config={"model_name": "baz"}) + rule_with_config = rule_with_config(config) + assert rule_with_config.severity == Severity.CRITICAL + assert rule_with_config.default_config == {"model_name": "model1"} + assert rule_with_config.config == {"model_name": "baz"} def test_get_config_file(): """Test that the config file is found in the current directory.""" directory = Path(__file__).parent / "resources" config = Config() - config.get_config_file(directory) - assert config.config_file == directory / "pyproject.toml" + config_file = config.get_config_file(directory) + assert config_file == directory / "pyproject.toml" def test_get_parent_config_file(): """Test that the config file is found in the parent directory.""" directory = Path(__file__).parent / "resources" / "sub_dir" config = Config() - config.get_config_file(directory) - assert config.config_file == directory.parent / "pyproject.toml" + config_file = config.get_config_file(directory) + assert config_file == directory.parent / "pyproject.toml" diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index 25b85ef..bff93d3 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -136,8 +136,8 @@ def test_evaluation_no_model_no_rule(manifest_empty_path, default_config): assert list(evaluation.scores.values()) == [] -def test_evaluation_rule_with_params( - manifest_path, rule_with_params, valid_config_path +def test_evaluation_rule_with_config( + manifest_path, rule_with_config, valid_config_path ): """Test rule evaluation with parameters.""" manifest_loader = ManifestLoader(manifest_path) @@ -148,7 +148,7 @@ def test_evaluation_rule_with_params( config._load_toml_file(str(valid_config_path)) rule_registry = RuleRegistry(config) - rule_registry._add_rule("rule_with_params", rule_with_params) + rule_registry._add_rule("rule_with_config", rule_with_config) rule_registry.init_rules() evaluation = Evaluation( @@ -160,8 +160,8 @@ def test_evaluation_rule_with_params( evaluation.evaluate() assert ( - rule_with_params.default_params - != rule_registry.rules["rule_with_params"].params + rule_with_config.default_config + != rule_registry.rules["rule_with_config"].config ) - assert evaluation.results[model1][rule_with_params] is not None - assert evaluation.results[model2][rule_with_params] is None + assert evaluation.results[model1][rule_with_config] is not None + assert evaluation.results[model2][rule_with_config] is None From 4af49711e9f802bccec2d453de287046f65a44f4 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Tue, 14 May 2024 11:33:37 +0200 Subject: [PATCH 28/29] Use rule.source() instead of reconstructing name --- src/dbt_score/rule_registry.py | 12 ++++++------ tests/resources/pyproject.toml | 4 ++-- tests/test_config.py | 2 +- tests/test_evaluation.py | 18 +++++++++--------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/dbt_score/rule_registry.py b/src/dbt_score/rule_registry.py index bc0c567..97b0fca 100644 --- a/src/dbt_score/rule_registry.py +++ b/src/dbt_score/rule_registry.py @@ -57,14 +57,14 @@ def _load(self, namespace_name: str) -> None: for obj_name in dir(module): obj = module.__dict__[obj_name] if type(obj) is type and issubclass(obj, Rule) and obj is not Rule: - self._add_rule(f"{module_name}.{obj_name}", obj) + self._add_rule(obj) - def _add_rule(self, name: str, rule: Type[Rule]) -> None: + def _add_rule(self, rule: Type[Rule]) -> None: """Add a rule.""" - if name in self._rules: - raise DuplicatedRuleException(name) - if name not in self.config.disabled_rules: - self._rules[name] = rule + if rule.source() in self._rules: + raise DuplicatedRuleException(rule.source()) + if rule.source() not in self.config.disabled_rules: + self._rules[rule.source()] = rule def load_all(self) -> None: """Load all rules, core and third-party.""" diff --git a/tests/resources/pyproject.toml b/tests/resources/pyproject.toml index 8db8981..511c0ce 100644 --- a/tests/resources/pyproject.toml +++ b/tests/resources/pyproject.toml @@ -3,10 +3,10 @@ rule_namespaces = ["namespace_foo"] disabled_rules = ["foo", "bar"] -[tool.dbt-score.rules.foobar] +[tool.dbt-score.rules."foo.bar"] severity=4 -[tool.dbt-score.rules.rule_with_config] +[tool.dbt-score.rules."tests.conftest.rule_with_config"] model_name="model2" [tool.dbt-score.rules."tests.rules.example.rule_test_example"] diff --git a/tests/test_config.py b/tests/test_config.py index 24a9ae0..ec646bf 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -12,7 +12,7 @@ def test_load_valid_toml_file(valid_config_path): config._load_toml_file(str(valid_config_path)) assert config.rule_namespaces == ["namespace_foo"] assert config.disabled_rules == ["foo", "bar"] - assert config.rules_config["foobar"].severity == Severity.CRITICAL + assert config.rules_config["foo.bar"].severity == Severity.CRITICAL assert ( config.rules_config["tests.rules.example.rule_test_example"].severity == Severity.CRITICAL diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index bff93d3..99dd136 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -23,10 +23,10 @@ def test_evaluation_low_medium_high( mock_scorer = Mock() rule_registry = RuleRegistry(default_config) - rule_registry._add_rule("rule_severity_low", rule_severity_low) - rule_registry._add_rule("rule_severity_medium", rule_severity_medium) - rule_registry._add_rule("rule_severity_high", rule_severity_high) - rule_registry._add_rule("rule_error", rule_error) + rule_registry._add_rule(rule_severity_low) + rule_registry._add_rule(rule_severity_medium) + rule_registry._add_rule(rule_severity_high) + rule_registry._add_rule(rule_error) rule_registry.init_rules() evaluation = Evaluation( @@ -64,8 +64,8 @@ def test_evaluation_critical( manifest_loader = ManifestLoader(manifest_path) rule_registry = RuleRegistry(default_config) - rule_registry._add_rule("rule_severity_low", rule_severity_low) - rule_registry._add_rule("rule_severity_critical", rule_severity_critical) + rule_registry._add_rule(rule_severity_low) + rule_registry._add_rule(rule_severity_critical) rule_registry.init_rules() evaluation = Evaluation( @@ -104,7 +104,7 @@ def test_evaluation_no_model(manifest_empty_path, rule_severity_low, default_con manifest_loader = ManifestLoader(manifest_empty_path) rule_registry = RuleRegistry(default_config) - rule_registry._add_rule("rule_severity_low", rule_severity_low) + rule_registry._add_rule(rule_severity_low) evaluation = Evaluation( rule_registry=rule_registry, @@ -148,7 +148,7 @@ def test_evaluation_rule_with_config( config._load_toml_file(str(valid_config_path)) rule_registry = RuleRegistry(config) - rule_registry._add_rule("rule_with_config", rule_with_config) + rule_registry._add_rule(rule_with_config) rule_registry.init_rules() evaluation = Evaluation( @@ -161,7 +161,7 @@ def test_evaluation_rule_with_config( assert ( rule_with_config.default_config - != rule_registry.rules["rule_with_config"].config + != rule_registry.rules["tests.conftest.rule_with_config"].config ) assert evaluation.results[model1][rule_with_config] is not None assert evaluation.results[model2][rule_with_config] is None From 87998dbd932c7f4f30cf9265ce6b23cf523d1e56 Mon Sep 17 00:00:00 2001 From: Jochem van Dooren Date: Tue, 14 May 2024 12:07:05 +0200 Subject: [PATCH 29/29] Use proper namespaces in tests --- tests/resources/pyproject.toml | 4 ++-- tests/test_config.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/resources/pyproject.toml b/tests/resources/pyproject.toml index 511c0ce..ee8d495 100644 --- a/tests/resources/pyproject.toml +++ b/tests/resources/pyproject.toml @@ -1,6 +1,6 @@ [tool.dbt-score] -rule_namespaces = ["namespace_foo"] -disabled_rules = ["foo", "bar"] +rule_namespaces = ["foo", "tests"] +disabled_rules = ["foo.foo", "tests.bar"] [tool.dbt-score.rules."foo.bar"] diff --git a/tests/test_config.py b/tests/test_config.py index ec646bf..a29a683 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -10,8 +10,8 @@ def test_load_valid_toml_file(valid_config_path): """Test that a valid config file loads correctly.""" config = Config() config._load_toml_file(str(valid_config_path)) - assert config.rule_namespaces == ["namespace_foo"] - assert config.disabled_rules == ["foo", "bar"] + assert config.rule_namespaces == ["foo", "tests"] + assert config.disabled_rules == ["foo.foo", "tests.bar"] assert config.rules_config["foo.bar"].severity == Severity.CRITICAL assert ( config.rules_config["tests.rules.example.rule_test_example"].severity