feat: ✨ integrate `check-datapackage` #1605

martonvago · 2025-11-21T09:23:35Z

Description

This PR integrates cdp 🎉 .
Says many files, but most were just deleted 🔥 .

Closes #1582

Needs an in-depth review.

Checklist

Formatted Markdown
Ran just run-all

martonvago · 2025-11-21T09:59:28Z

src/seedcase_sprout/check_properties.py

+    package_required_checks = _map(
+        PACKAGE_SPROUT_REQUIRED_FIELDS,
+        lambda field: cdp.RequiredCheck(
+            jsonpath=f"$.{field}",
+            message=f"'{field}' is a required property",
+        ),
+    )


Some required fields are required already in cdp, but I thought not separating those out looked cleaner. Issues will be deduplicated anyway.

One thing to note about deduplication is that for an issue to count as a duplicate all properties must be the same, including message. This actually places a restriction on the message format, especially in RequiredChecks, because the format is determined by jsonschema. To prevent bugs, maybe we should

not include the message when deciding if two issues are the same (at least for type="required")

or autoformat the message inside RequiredCheck?

Not sure how explain will influence this.

martonvago · 2025-11-21T11:05:13Z

src/seedcase_sprout/check_properties.py

+    not_blank_resource_fields = _map(
+        RESOURCE_SPROUT_REQUIRED_FIELDS, lambda field: f"$.resources[*].{field}"
+    )
+    not_blank = cdp.CustomCheck(
+        jsonpath=(
+            f"$.{' | '.join(PACKAGE_SPROUT_REQUIRED_FIELDS)} "
+            "| $.contributors[*].title"
+            "| $.sources[*].title"
+            "| $.licenses[*].name"
+            "| $.licenses[*].path"
+            f"| {' | '.join(not_blank_resource_fields)}"
+        ),
+        message="This property must not be empty.",
+        check=lambda value: bool(value),
+        type="not-blank",
+    )


Could also create a separate CustomCheck for each path instead of the union

martonvago · 2025-11-21T11:07:56Z

src/seedcase_sprout/check_properties.py

+        type="resource-path-string",
+    )
+    resource_path_format = cdp.CustomCheck(
+        jsonpath="$.resources[*]",


Targeting the entire resource because in the check function below I need access to the resource name to check the path.

The downside is that the JSON path in the issue will be, e.g., $.resources[0], and not $.resources[0].path.

martonvago · 2025-11-21T11:32:49Z

src/seedcase_sprout/check_properties.py

+    no_resource_data = cdp.CustomCheck(
+        jsonpath="$.resources[*].data",
+        message=(
+            "Sprout doesn't use the `data` field, instead it expects data "
+            "in separate files that are given in the `path` field."
+        ),
+        check=lambda value: value is None,
+        type="no-inline-data",
+    )


I added this, but, because there is no data attribute in ResourceProperties in Sprout, even if data is added manually, compact_dict will not include it in the dict representation. So, even if data is present in datapackage.json, it will not be present in the properties passed to cdp.check. So no_resource_data will never fail.

Should we do something about this?

martonvago · 2025-11-21T11:38:58Z

src/seedcase_sprout/check_properties.py

+    exclude_resource_path_or_data_required = cdp.Exclusion(
+        jsonpath="$.resources[*]", type="required"
+    )


Only matches the case when both path and data are missing. Ignoring because we don't want any feedback on data. The missing path will be flagged separately.

martonvago · 2025-11-21T11:41:02Z

src/seedcase_sprout/check_properties.py

+    exclude_resource_path_type = cdp.Exclusion(
+        jsonpath="$.resources[*].path", type="type"
+    )


According to the DP standard, path can be string or array. If path is another type, the feedback will reference both string and array. In Sprout, we only allow string paths, so I'm excluding this check. That path is a string will be checked separately.

martonvago · 2025-11-21T11:41:48Z

src/seedcase_sprout/check_properties.py

+    exclude_resource_path_pattern = cdp.Exclusion(
+        jsonpath="$.resources[*].path", type="pattern"
+    )


Same reasoning as above: Sprout has its own rules for what paths should look like.

martonvago · 2025-11-21T11:43:36Z

src/seedcase_sprout/check_properties.py

+    exclude_resource_path_min_items = cdp.Exclusion(
+        jsonpath="$.resources[*].path", type="minItems"
+    )


If path is an array, we are already flagging it as not a string (the Sprout rule), so I'm excluding specific feedback about bad arrays.

martonvago · 2025-11-21T11:46:09Z

src/seedcase_sprout/check_properties.py

    return properties


+def _check_resource_path_format(resource_properties: Any) -> bool:


Moved from get_sprout_resource_errors.py

martonvago · 2025-11-21T11:47:48Z

pyproject.toml

 build-backend = "hatchling.build"
+
+[tool.uv.sources]
+check-datapackage = { git = "https://github.com/seedcase-project/check-datapackage" }


Hope this is correct for the time being

martonvago · 2025-11-21T11:48:37Z

tests/sprout_checks/test_check_properties.py

-    # Even when resources isn't there.
-    delattr(properties, "resources")
-    assert check_properties(properties) == properties


Moved to test below

martonvago · 2025-11-21T11:56:12Z

tests/sprout_checks/test_check_properties.py

+    # All properties checks
+    with raises(cdp.DataPackageError) as error1:
+        check_properties(properties)
+    assert "`data`" not in str(error1.value)


Asserts of this style are imo pretty brittle and imprecise, but I really wanted to test each config item that we define in Sprout. It took multiple days to write them and the only way I could keep track of them working together in the expected way was by repeatedly running these tests. So that feels like a good indication that these tests are useful.

martonvago · 2025-11-28T10:28:24Z

src/seedcase_sprout/check_properties.py

+        data_package_error: cdp.DataPackageError,
+    ) -> None:
+        """Create a `DataResourceError` from a `cdp.DataPackageError`."""
+        message = str(data_package_error).replace("package.resources[0]", "resource")


Don't think this can be done more intelligently at this point, but it would be nicer if I wasn't replacing blindly.

Example output:

E seedcase_sprout.check_properties.DataResourceError: 4 issues were found in your `datapackage.json`: E E At resource.description: E | E | description: None E | ^^^^ E 'description' is a required property

Should we change/remove the reference to datapackage.json?

Also, there is something slightly off with instance, but we can look at that as part of another PR.

martonvago added 4 commits November 20, 2025 10:32

feat: ✨ integrate cdp into check_properties

8ba3443

test: ✅ update tests

be2960d

fix: 🐛 fix several outsanding issues with the config

1f674bd

chore: 🔧 update lockfile and vulture list

de4ffd4

martonvago self-assigned this Nov 21, 2025

martonvago added this to Iteration planning Nov 21, 2025

github-project-automation bot moved this to Todo in Iteration planning Nov 21, 2025

martonvago added 4 commits November 21, 2025 09:26

chore: 🔥 remove old cdp code

2e12937

Merge branch 'main' into feat/integrate-cdp

cf9efd9

docs: 📝 no raw string needed

05a4087

fix: 🐛 hide traceback from DataPackageError

b386ae2

martonvago commented Nov 21, 2025

View reviewed changes

martonvago moved this from Todo to In Review in Iteration planning Nov 21, 2025

Merge branch 'main' into feat/integrate-cdp

904b847

martonvago marked this pull request as ready for review November 21, 2025 12:05

martonvago requested a review from a team as a code owner November 21, 2025 12:05

martonvago added 3 commits November 28, 2025 09:39

Merge branch 'main' into feat/integrate-cdp

71ba81d

refactor: ♻️ add cdp dependency from pypi

b54153b

refactor: ♻️ update after explain implemented

97687ed

martonvago commented Nov 28, 2025

View reviewed changes

		return properties


		def _check_resource_path_format(resource_properties: Any) -> bool:

feat: ✨ integrate check-datapackage #1605

Are you sure you want to change the base?

feat: ✨ integrate check-datapackage #1605

Conversation

martonvago commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

martonvago Nov 21, 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!

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!

martonvago Nov 21, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat: ✨ integrate `check-datapackage` #1605

feat: ✨ integrate `check-datapackage` #1605

martonvago commented Nov 21, 2025 •

edited

Loading

martonvago Nov 21, 2025 •

edited

Loading

martonvago Nov 21, 2025 •

edited

Loading