Skip to content

Commit

Permalink
refactor ♻️ allow properties input in checks (#999)
Browse files Browse the repository at this point in the history
## Description

This PR makes the suggestion of having check functions in Sprout accept
both `dict`s and `Properties` objects.
I thought of this when I was writing up the guide for check functions
and kept having to transform each `Properties` object into a `dict`
before using it in an example.

Not strictly necessary and maybe we don't want it, but here it is
:1st_place_medal:

<!-- Select quick/in-depth as necessary -->
This PR needs a medium-depth review.

## Checklist

- [x] Added or updated tests
- [x] Updated documentation
- [x] Ran `just run-all`
  • Loading branch information
martonvago authored Jan 28, 2025
1 parent 02bc68e commit d22a7a1
Show file tree
Hide file tree
Showing 10 changed files with 50 additions and 19 deletions.
3 changes: 1 addition & 2 deletions seedcase_sprout/core/create_resource_properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,4 @@ def create_resource_properties(
"""
check_is_dir(path)
properties.path = str(create_relative_resource_data_path(path))
check_resource_properties(properties.compact_dict)
return properties
return check_resource_properties(properties)
6 changes: 2 additions & 4 deletions seedcase_sprout/core/edit_package_properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,9 @@ def edit_package_properties(
)
```
"""
properties = properties.compact_dict

check_is_file(path)

properties.pop("resources", None)
properties.resources = None
check_package_properties(
properties, ignore=[CheckErrorMatcher(validator="required")]
)
Expand All @@ -90,7 +88,7 @@ def edit_package_properties(
ignore=[CheckErrorMatcher(validator="required")],
)

current_properties.update(properties)
current_properties.update(properties.compact_dict)

check_properties(
current_properties,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
from seedcase_sprout.core.checks.check_error_matcher import CheckErrorMatcher
from seedcase_sprout.core.properties import PackageProperties
from seedcase_sprout.core.sprout_checks.check_properties import (
RESOURCE_FIELD_PATTERN,
check_properties,
)


def check_package_properties(
properties: dict, ignore: list[CheckErrorMatcher] = []
) -> dict:
properties: PackageProperties | dict, ignore: list[CheckErrorMatcher] = []
) -> PackageProperties | dict:
"""Checks that package `properties` matches requirements in Sprout.
`properties` is checked against the Data Package standard and the following
Expand Down
19 changes: 14 additions & 5 deletions seedcase_sprout/core/sprout_checks/check_properties.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from seedcase_sprout.core import checks
from seedcase_sprout.core.checks.check_error_matcher import CheckErrorMatcher
from seedcase_sprout.core.checks.exclude_matching_errors import exclude_matching_errors
from seedcase_sprout.core.properties import PackageProperties
from seedcase_sprout.core.sprout_checks.get_sprout_package_errors import (
get_sprout_package_errors,
)
Expand All @@ -11,7 +12,9 @@
RESOURCE_FIELD_PATTERN = r"resources\[\d+\]"


def check_properties(properties: dict, ignore: list[CheckErrorMatcher] = []) -> dict:
def check_properties(
properties: PackageProperties | dict, ignore: list[CheckErrorMatcher] = []
) -> PackageProperties | dict:
"""Checks that `properties` matches requirements in Sprout.
`properties` is checked against the Data Package standard and the following
Expand All @@ -34,11 +37,17 @@ def check_properties(properties: dict, ignore: list[CheckErrorMatcher] = []) ->
Raises:
ExceptionGroup: A group of `CheckError`s, one error per failed check.
"""
errors = checks.check_properties(properties)
errors += get_sprout_package_errors(properties)
properties_dict = (
properties.compact_dict
if isinstance(properties, PackageProperties)
else properties
)

errors = checks.check_properties(properties_dict)
errors += get_sprout_package_errors(properties_dict)

if isinstance(properties.get("resources"), list):
for index, resource in enumerate(properties.get("resources")):
if isinstance(properties_dict.get("resources"), list):
for index, resource in enumerate(properties_dict.get("resources")):
errors += get_sprout_resource_errors(resource, index)

errors = exclude_matching_errors(
Expand Down
13 changes: 10 additions & 3 deletions seedcase_sprout/core/sprout_checks/check_resource_properties.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
from seedcase_sprout.core.checks.check_error_matcher import CheckErrorMatcher
from seedcase_sprout.core.properties import ResourceProperties
from seedcase_sprout.core.sprout_checks.check_properties import check_properties

PACKAGE_FIELD_PATTERN = r"\$\.\w+$"


def check_resource_properties(
properties: dict, ignore: list[CheckErrorMatcher] = []
) -> dict:
properties: ResourceProperties | dict, ignore: list[CheckErrorMatcher] = []
) -> ResourceProperties | dict:
"""Checks that resource `properties` matches requirements in Sprout.
`properties` is checked against the Data Package standard and the following
Expand All @@ -29,9 +30,15 @@ def check_resource_properties(
Raises:
ExceptionGroup: A group of `CheckError`s, one error per failed check.
"""
properties_dict = (
properties.compact_dict
if isinstance(properties, ResourceProperties)
else properties
)

try:
check_properties(
{"resources": [properties]},
{"resources": [properties_dict]},
ignore=[*ignore, CheckErrorMatcher(json_path=PACKAGE_FIELD_PATTERN)],
)
except ExceptionGroup as error_info:
Expand Down
3 changes: 1 addition & 2 deletions seedcase_sprout/core/write_package_properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ def write_package_properties(path: Path, properties: PackageProperties) -> Path:
ExceptionGroup: If there is an error in the properties. A group of
`CheckError`s, one error for each failed check.
"""
properties = properties.compact_dict
check_properties(
properties,
ignore=[CheckErrorMatcher(validator="required", json_path=r"resources$")],
)
return write_json(properties, path)
return write_json(properties.compact_dict, path)
2 changes: 1 addition & 1 deletion seedcase_sprout/core/write_resource_properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ def write_resource_properties(
# )
```
"""
resource_properties = resource_properties.compact_dict
check_is_file(path)
check_resource_properties(resource_properties)

Expand All @@ -81,6 +80,7 @@ def write_resource_properties(
ignore=[CheckErrorMatcher(validator="required", json_path="resources")],
)

resource_properties = resource_properties.compact_dict
resource_id = get_resource_id_from_properties(resource_properties)
current_resource = get_resource_properties(package_properties, resource_id)
if current_resource:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ def test_passes_full_package_properties(properties):
assert check_package_properties(properties) == properties


def test_check_accepts_properties_object(properties):
"""Should accept a properties object as input."""
properties = PackageProperties.from_dict(properties)
assert check_package_properties(properties) == properties


@mark.parametrize("resources", [[{}], [{"name": "name", "path": "data.csv"}]])
def test_passes_without_checking_resources(resources, properties):
"""Should pass well-formed package properties without checking individual resource
Expand Down
6 changes: 6 additions & 0 deletions tests/core/sprout_checks/test_check_properties_sprout.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ def test_check_passes_full_properties(properties):
assert check_properties(properties) == properties


def test_check_accepts_properties_object(properties):
"""Should accept a properties object as input."""
properties = PackageProperties.from_dict(properties)
assert check_properties(properties) == properties


@mark.parametrize("field", [*PACKAGE_SPROUT_REQUIRED_FIELDS.keys(), "resources"])
def test_raises_error_if_package_required_field_is_missing(properties, field):
"""Should raise an error if a required field is missing among the package
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ def test_passes_full_resource_properties(properties):
assert check_resource_properties(properties) == properties


def test_check_accepts_properties_object(properties):
"""Should accept a properties object as input."""
properties = ResourceProperties.from_dict(properties)
assert check_resource_properties(properties) == properties


@mark.parametrize(
"field",
RESOURCE_SPROUT_REQUIRED_FIELDS.keys(),
Expand Down

0 comments on commit d22a7a1

Please sign in to comment.