Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jochemvandooren committed May 3, 2024
1 parent 12d49fc commit ae091fe
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 49 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python: ["3.11"]
python: ["3.11", "3.12"]

steps:
- uses: actions/checkout@v4
Expand Down
29 changes: 1 addition & 28 deletions pdm.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions src/dbt_score/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()
Expand Down
29 changes: 24 additions & 5 deletions src/dbt_score/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand All @@ -28,20 +29,21 @@ 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:
"""Initialize the Config object."""
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)
Expand All @@ -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."
)
Expand All @@ -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))
9 changes: 5 additions & 4 deletions src/dbt_score/rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,17 @@ 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."""
super().__init_subclass__(**kwargs)
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()

Expand All @@ -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."""
Expand Down
8 changes: 4 additions & 4 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
)
Expand All @@ -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
Expand All @@ -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)
21 changes: 19 additions & 2 deletions tests/test_config.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Tests for the module config_parser."""
from pathlib import Path

import pytest
from dbt_score.config import Config, RuleConfig
Expand All @@ -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
Expand All @@ -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


Expand All @@ -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"
2 changes: 1 addition & 1 deletion tests/test_evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_rule_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit ae091fe

Please sign in to comment.