Add support for linting and scoring dbt seeds #110

StrahilPeykov · 2025-04-15T11:37:50Z

Overview

Add support for linting and scoring seed resources in dbt-score, following issue #105.

Problem

Previously, dbt-score only supported linting models, sources, and snapshots. Seeds were not evaluated, creating an inconsistency in the quality assessment of a dbt project's metadata. Since seeds often contain important reference data, ensuring they have proper documentation and ownership is valuable.

Implementation

Added Seed class to represent dbt seeds
Updated ManifestLoader to load seeds from the manifest
Added seed-specific linting rules (description, columns, tests, ownership)
Updated Evaluation class to include seeds in evaluation chain
Modified formatters to handle and display seed results
Added comprehensive tests for seed support

New Rules

seed_has_description - Ensures seeds have descriptive documentation
seed_columns_have_description - Verifies seed columns are documented
seed_has_tests - Checks that seeds have appropriate tests
seed_has_owner - Ensures seeds have defined ownership

Testing

Full test coverage has been added for seed support.

Fixtures for seeds in test suite
Tests for seed-specific rules
Updates to existing tests to accommodate seeds

jochemvandooren · 2025-04-17T12:10:28Z

Thanks for opening a PR 🙌 , I will have a look soon. There's some linting errors by the way: https://github.com/PicnicSupermarket/dbt-score/actions/runs/14473008011/job/40645749304?pr=110

jochemvandooren

Thanks for your contribution! 🙌 Overall, the feature looks very good, I will have a closer look at the tests tomorrow. Left some small comments already

CHANGELOG.md

docs/seed.md

jochemvandooren · 2025-04-22T16:11:28Z

src/dbt_score/rules/generic.py

+    if invalid_column_names:
+        max_length = 60
+        message = f"Columns lack a description: {', '.join(invalid_column_names)}."
+        if len(message) > max_length:


Isn't this redundant, as you can also do f"{message[:60]}…" if the length is lower than 60? It will just show the full string I think

src/dbt_score/rules/generic.py

jochemvandooren · 2025-04-22T16:25:33Z

Also, there's still some linting errors related to mypy! You can run pre-commit run --all-files locally to get those errors

StrahilPeykov · 2025-04-23T10:04:48Z

Thanks a lot for the feedback @jochemvandooren! I've addressed all your comments - updated the PR number in the CHANGELOG, removed the seed.md documentation file for consistency, simplified the max_length check in the column description rule, changed the severity of seed_has_tests to LOW, and fixed all the mypy and linting issues with pre-commit.

jochemvandooren

Just left some final comments on the tests, great work and thanks for improving the tests! 🙌

jochemvandooren · 2025-04-24T07:15:22Z

tests/test_cli.py

 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.lint_dbt_project") as mock_lint:


Why do we do this here? Now we are patching lint_dbt_project, meaning we will not actually lint the manifest, which was the goal of the test.

jochemvandooren · 2025-04-24T07:18:43Z

tests/test_cli.py

+    mock_eval.project_score = Score(5.0, "🥉")  # Score below 10.0
+    mock_eval.scores.values.return_value = []
+
+    with patch("dbt_score.cli.lint_dbt_project") as mock_lint:


I think here it makes more sense! Now we actually test only the fail_project_under behavior 👍

jochemvandooren · 2025-04-24T07:19:05Z

tests/test_cli.py

+
+    with patch("dbt_score.cli.lint_dbt_project") as mock_lint:
+        mock_lint.return_value = mock_eval
+        # Also patch the HumanReadableFormatter to control the output


Stray comment?

jochemvandooren · 2025-04-24T07:19:39Z

tests/test_cli.py

        assert result.exit_code == 1


 def test_fail_any_model_under(manifest_path):


Suggested change

def test_fail_any_model_under(manifest_path):

def test_fail_any_item_under(manifest_path):

Consistency 🤓

jochemvandooren · 2025-04-24T07:23:44Z

tests/test_seed_rules.py

This is great 👌 We should do the same for other dbt entities! Will look into it in another PR

src/dbt_score/rules/generic.py

Co-authored-by: Jochem van Dooren <[email protected]>

jochemvandooren

Nice, thanks a lot! 🙌 I'll leave the last discussion up to you and @matthieucan

jochemvandooren · 2025-04-30T07:12:30Z

Ah I have merged another PR, I am afraid you have to resolve some conflicts. Please let me know if you need any help there!

jochemvandooren

Great to see the rebasing worked out 👌 I suggest try keeping the changes related to the seed feature only! Left a couple of comments about that

CHANGELOG.md

jochemvandooren · 2025-05-02T12:49:54Z

src/dbt_score/models.py

+    def get_first_model(self) -> Model | None:
+        """Get the first model in the collection, if any."""
+        return next(iter(self.models.values())) if self.models else None
+
+    def get_first_source(self) -> Source | None:
+        """Get the first source in the collection, if any."""
+        return next(iter(self.sources.values())) if self.sources else None
+
+    def get_first_snapshot(self) -> Snapshot | None:
+        """Get the first snapshot in the collection, if any."""
+        return next(iter(self.snapshots.values())) if self.snapshots else None
+
+    def get_first_seed(self) -> Seed | None:
+        """Get the first seed in the collection, if any."""
+        return next(iter(self.seeds.values())) if self.seeds else None


I don't think these methods serve any purpose, other than being used in the tests. So I suggest not creating these

Actually the case for all these helper functions

jochemvandooren · 2025-05-02T12:53:04Z

src/dbt_score/models.py

+        elif parent_id in self.seeds:
+            node.parents.append(self.seeds[parent_id])
+
+    def _populate_parents(self) -> None:


Why was this method changed? I try to keep the changes related to seeds and not change to much related to other functionalities to keep things small and related to a single feature! Also this code was reviewed, approved and merged so I see no reason to change it unless you have very good reasons to, does that make sense?

Yeah you're right, I added them for convenience for tests, but I have now removed them

Co-authored-by: Jochem van Dooren <[email protected]>

…ov/dbt-score into feature/seed-support

jochemvandooren

Some final comment! 🙌

jochemvandooren · 2025-05-02T13:24:01Z

tests/test_evaluation.py


    evaluation.evaluate()

-    model2 = manifest_loader.models["model.package.model2"]


i don't see what's wrong with fetching the model from the manifest by it's ID? I think this is a neater way of doing it than having to search it by name?

Same for the other ocurrences, i think fetching the model by it's key should be the best way to do it!

jochemvandooren · 2025-05-02T13:28:15Z

tests/test_models.py

+
+
+@patch("dbt_score.models.Path.read_text")
+def test_parent_references(mock_read_text, raw_manifest):


I think a test is already in place for this: https://github.com/ross-whatnot/dbt-score/blob/edf0563d0f93aecd41a639ccdb628eff5ae8aded/tests/test_models.py#L38-L49

src/dbt_score/rules/generic.py

jochemvandooren · 2025-05-06T07:16:29Z

Awesome @StrahilPeykov, thanks a lot for your great contribution! 🙌

StrahilPeykov added 4 commits April 15, 2025 13:34

Add support for seeds in dbt-score

ac94d79

changelog

de07dec

Fix markdown formatting for prettier

eb311d8

Fix markdown formatting for prettier

7c4b924

fix linting errors

eaf268a

jochemvandooren mentioned this pull request Apr 22, 2025

Exposures and children #111

Closed

jochemvandooren reviewed Apr 22, 2025

View reviewed changes

jochemvandooren requested review from druzhinin-kirill and matthieucan April 22, 2025 16:27

address PR feedback for seed support

ede7146

jochemvandooren reviewed Apr 24, 2025

View reviewed changes

matthieucan reviewed Apr 24, 2025

View reviewed changes

src/dbt_score/rules/generic.py Outdated Show resolved Hide resolved

src/dbt_score/rules/generic.py Outdated Show resolved Hide resolved

src/dbt_score/rules/generic.py Outdated Show resolved Hide resolved

StrahilPeykov and others added 4 commits April 25, 2025 11:35

Update tests/test_cli.py

b5c10f0

Co-authored-by: Jochem van Dooren <[email protected]>

remove seed_has_tests

b8b2b14

access seed owner from config.meta instead of meta

6834470

fix test_cli to properly test manifest linting

60415f0

jochemvandooren approved these changes Apr 28, 2025

View reviewed changes

jochemvandooren mentioned this pull request Apr 30, 2025

Embedding parents into models and snapshots #109

Merged

StrahilPeykov added 4 commits May 1, 2025 13:01

convert from lists to dictionaries

654a9e6

parent relationships

67849ea

wording

0446991

Merge branch 'master' into feature/seed-support

4219d0d

jochemvandooren reviewed May 2, 2025

View reviewed changes

StrahilPeykov and others added 3 commits May 2, 2025 15:01

Update CHANGELOG.md

4616518

Co-authored-by: Jochem van Dooren <[email protected]>

remove helper methods

8c0bfe3

Merge branch 'feature/seed-support' of https://github.com/StrahilPeyk…

49947dd

…ov/dbt-score into feature/seed-support

jochemvandooren reviewed May 2, 2025

View reviewed changes

StrahilPeykov added 3 commits May 2, 2025 15:40

Update test_models.py

79f130b

direct lookups

8eb8ba4

constant

5f2fd94

matthieucan reviewed May 2, 2025

View reviewed changes

src/dbt_score/rules/generic.py Outdated Show resolved Hide resolved

Update generic.py

7bbe99f

jochemvandooren merged commit cb57712 into PicnicSupermarket:master May 6, 2025
4 checks passed

jochemvandooren linked an issue May 6, 2025 that may be closed by this pull request

Add support for seeds #105

Closed

jochemvandooren mentioned this pull request May 6, 2025

Adding children in addition to parents #113

Merged

matthieucan mentioned this pull request May 8, 2025

Adding support for exposures #112

Merged

		assert result.exit_code == 1


		def test_fail_any_model_under(manifest_path):

	def test_fail_any_model_under(manifest_path):
	def test_fail_any_item_under(manifest_path):


		evaluation.evaluate()

		model2 = manifest_loader.models["model.package.model2"]



		@patch("dbt_score.models.Path.read_text")
		def test_parent_references(mock_read_text, raw_manifest):

Add support for linting and scoring dbt seeds #110

Add support for linting and scoring dbt seeds #110

Uh oh!

Conversation

StrahilPeykov commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Problem

Implementation

New Rules

Testing

Uh oh!

jochemvandooren commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jochemvandooren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jochemvandooren commented Apr 22, 2025

Uh oh!

StrahilPeykov commented Apr 23, 2025

Uh oh!

jochemvandooren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jochemvandooren Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jochemvandooren left a comment

Choose a reason for hiding this comment

Uh oh!

jochemvandooren commented Apr 30, 2025

Uh oh!

jochemvandooren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jochemvandooren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jochemvandooren commented May 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

StrahilPeykov commented Apr 15, 2025 •

edited

Loading

jochemvandooren commented Apr 17, 2025 •

edited

Loading

jochemvandooren Apr 24, 2025 •

edited

Loading