Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ✨ add simple helper functions #962

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
55c03c6
refactor: :recycle: use custom CheckError in checks
martonvago Dec 18, 2024
958bb18
feat: :sparkles: improve CheckError
martonvago Dec 19, 2024
a619ded
test: :white_check_mark: add tests for helper functions
martonvago Dec 19, 2024
3586c51
refactor: :recycle: return early from function
martonvago Dec 19, 2024
d3de146
docs: :memo: update test docstrings
martonvago Dec 19, 2024
05e7d41
apply suggestions from code review
martonvago Jan 10, 2025
52d3a24
chore(pre-commit): :pencil2: automatic fixes
pre-commit-ci[bot] Jan 10, 2025
64d7a87
Merge branch 'main' into feat/check-error
martonvago Jan 10, 2025
8196dc0
feat: :sparkles: put required fields into constants
martonvago Jan 10, 2025
8c701d8
feat: :sparkles: add simple helper functions
martonvago Jan 10, 2025
a08ce8b
apply suggestions from code review
martonvago Jan 13, 2025
1dd1e1e
refactor: :recycle: rename function to validation_errors_to_check_errors
martonvago Jan 13, 2025
c595aa9
refactor: :recycle: rename file to validation_errors_to_check_errors
martonvago Jan 13, 2025
cb49bd1
docs: :memo: add more detail to docstring
martonvago Jan 13, 2025
d603d82
refactor: :recycle: rename constant to PACKAGE_RECOMMENDED_FIELDS
martonvago Jan 14, 2025
abfc4c5
fix: :bug: include `data` in resource required fields
martonvago Jan 14, 2025
6611019
refactor: :recycle: make structure of PACKAGE_SPROUT_REQUIRED_FIELDS …
martonvago Jan 14, 2025
fea4a5f
refactor: :recycle: drop fields using pop
martonvago Jan 14, 2025
1cbec8a
apply suggestions from code review
martonvago Jan 14, 2025
c5b784f
refactor: :recycle: rename function to get_sprout_specific_resource_e…
martonvago Jan 14, 2025
c854a2d
refactor: :recycle: rename file to get_sprout_specific_resource_errors
martonvago Jan 14, 2025
ee5edf0
docs: :memo: update test names and docstrings
martonvago Jan 14, 2025
ee05805
Merge branch 'feat/check-error' into feat/sprout-checks-1-required-fi…
martonvago Jan 14, 2025
e43d13e
Merge branch 'feat/sprout-checks-1-required-fields' into feat/sprout-…
martonvago Jan 14, 2025
569b947
refactor: :recycle: rename function to exclude_non_sprout_resource_er…
martonvago Jan 14, 2025
87418fb
refactor: :recycle: rename file to exclude_non_sprout_resource_errors
martonvago Jan 14, 2025
144b12f
apply suggestions from code review
martonvago Jan 15, 2025
b33c387
Merge branch 'main' into feat/sprout-checks-2-simple-helper-functions
lwjohnst86 Jan 20, 2025
f0510d8
Merge branch 'main' into feat/sprout-checks-2-simple-helper-functions
lwjohnst86 Jan 20, 2025
7c19388
refactor: :recycle: rename functions
martonvago Jan 20, 2025
abc47e9
refactor: :recycle: rename files
martonvago Jan 20, 2025
a0d4897
refactor: :recycle: update error message and assertions
martonvago Jan 20, 2025
fd6282c
refactor: :recycle: pull out type error message into constant
martonvago Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions seedcase_sprout/core/checks/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from .check_properties import check_properties
from .check_resource_properties import check_resource_properties
from .required_fields import (
PACKAGE_RECOMMENDED_REQUIRED_FIELDS,
PACKAGE_RECOMMENDED_FIELDS,
PACKAGE_REQUIRED_FIELDS,
RESOURCE_REQUIRED_FIELDS,
RequiredFieldType,
Expand All @@ -16,7 +16,7 @@
"check_properties",
"check_package_properties",
"check_resource_properties",
"PACKAGE_RECOMMENDED_REQUIRED_FIELDS",
"PACKAGE_RECOMMENDED_FIELDS",
"PACKAGE_REQUIRED_FIELDS",
"RESOURCE_REQUIRED_FIELDS",
"RequiredFieldType",
Expand Down
4 changes: 2 additions & 2 deletions seedcase_sprout/core/checks/add_package_recommendations.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from seedcase_sprout.core.checks.config import NAME_PATTERN, SEMVER_PATTERN
from seedcase_sprout.core.checks.required_fields import (
PACKAGE_RECOMMENDED_REQUIRED_FIELDS,
PACKAGE_RECOMMENDED_FIELDS,
)


Expand All @@ -15,7 +15,7 @@ def add_package_recommendations(schema: dict) -> dict:
Returns:
The updated Data Package schema.
"""
schema["required"].extend(PACKAGE_RECOMMENDED_REQUIRED_FIELDS.keys())
schema["required"].extend(PACKAGE_RECOMMENDED_FIELDS.keys())
schema["properties"]["name"]["pattern"] = NAME_PATTERN
schema["properties"]["version"]["pattern"] = SEMVER_PATTERN
schema["properties"]["contributors"]["items"]["required"] = ["title"]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
from jsonschema import Draft7Validator, FormatChecker

from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.checks.to_check_errors import to_check_errors
from seedcase_sprout.core.checks.validation_errors_to_check_errors import (
validation_errors_to_check_errors,
)


def check_object_against_json_schema(
Expand All @@ -24,4 +26,4 @@ def check_object_against_json_schema(
"""
Draft7Validator.check_schema(schema)
validator = Draft7Validator(schema, format_checker=FormatChecker())
return to_check_errors(validator.iter_errors(json_object))
return validation_errors_to_check_errors(validator.iter_errors(json_object))
8 changes: 5 additions & 3 deletions seedcase_sprout/core/checks/get_full_json_path_from_error.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@


def get_full_json_path_from_error(error: ValidationError) -> str:
"""Returns a more complete `json_path` to the error, if possible.
"""Returns the full `json_path` to the error.

For 'required' errors, the field name is extracted from the error message.

Args:
error: The error.
error: The error to get the full `json_path` for.

Returns:
The `json_path`.
The full`json_path` of the error.
"""
if error.validator == "required":
match = re.search("'(.*)' is a required property", error.message)
Expand Down
4 changes: 3 additions & 1 deletion seedcase_sprout/core/checks/required_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ class RequiredFieldType(str, Enum):

str = "str"
list = "list"
any = "any"


PACKAGE_REQUIRED_FIELDS = {
"resources": RequiredFieldType.list,
}

PACKAGE_RECOMMENDED_REQUIRED_FIELDS = {
PACKAGE_RECOMMENDED_FIELDS = {
"name": RequiredFieldType.str,
"id": RequiredFieldType.str,
"licenses": RequiredFieldType.list,
Expand All @@ -23,4 +24,5 @@ class RequiredFieldType(str, Enum):
RESOURCE_REQUIRED_FIELDS = {
"name": RequiredFieldType.str,
"path": RequiredFieldType.str,
"data": RequiredFieldType.any,
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
COMPLEX_VALIDATORS = {"allOf", "anyOf", "oneOf"}


def to_check_errors(validation_errors: Iterator[ValidationError]) -> list[CheckError]:
def validation_errors_to_check_errors(
validation_errors: Iterator[ValidationError],
) -> list[CheckError]:
"""Transforms `jsonschema.ValidationError`s to more compact `CheckError`s.

The list of errors is:
Expand Down
21 changes: 10 additions & 11 deletions seedcase_sprout/core/sprout_checks/check_data_path_string.py
martonvago marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,13 @@ def check_data_path_string(
A list of errors. An empty list if no error was found.
"""
path = properties.get("path", "")
return (
[]
if type(path) is str
else [
CheckError(
message=f"{path} is not of type 'string'",
json_path=get_json_path_to_resource_field("path", index),
validator="type",
)
]
)
if isinstance(path, str):
return []

return [
CheckError(
message=f"{path} is not of type 'string'",
martonvago marked this conversation as resolved.
Show resolved Hide resolved
json_path=get_json_path_to_resource_field("path", index),
validator="type",
)
]
27 changes: 13 additions & 14 deletions seedcase_sprout/core/sprout_checks/check_no_inline_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,16 @@ def check_no_inline_data(
Returns:
A list of errors. The empty list if no error was found.
"""
return (
[]
if properties.get("data") is None
else [
CheckError(
message=(
"'data' should not be set. Sprout expects data in separate "
"data files specified by 'path'."
),
json_path=get_json_path_to_resource_field("data", index),
validator="inline-data",
)
]
)
if properties.get("data") is None:
return []

return [
CheckError(
message=(
"'data' should not be set. Sprout expects data in separate "
"data files specified by 'path'."
martonvago marked this conversation as resolved.
Show resolved Hide resolved
),
json_path=get_json_path_to_resource_field("data", index),
validator="inline-data",
)
]
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def check_resource_id_in_data_path(
The properties, if the data path is well-formed.
"""
data_path = properties.get("path")
if data_path is None or type(data_path) is not str:
if not isinstance(data_path, str):
return []

data_path = Path(data_path)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from seedcase_sprout.core.checks.check_error import CheckError


def remove_not_sprout_related_resource_errors(
def exclude_non_sprout_resource_errors(
errors: list[CheckError],
) -> list[CheckError]:
"""Filters out resource errors that are not relevant for Sprout.
Expand Down
18 changes: 12 additions & 6 deletions seedcase_sprout/core/sprout_checks/required_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,20 @@

# Sprout-specific required fields and their types

PACKAGE_SPROUT_REQUIRED_FIELDS = checks.PACKAGE_RECOMMENDED_REQUIRED_FIELDS | {
"title": RequiredFieldType.str,
"description": RequiredFieldType.str,
"version": RequiredFieldType.str,
"created": RequiredFieldType.str,
}
PACKAGE_SPROUT_REQUIRED_FIELDS = (
checks.PACKAGE_REQUIRED_FIELDS
| checks.PACKAGE_RECOMMENDED_FIELDS
| {
"title": RequiredFieldType.str,
"description": RequiredFieldType.str,
"version": RequiredFieldType.str,
"created": RequiredFieldType.str,
}
)
PACKAGE_SPROUT_REQUIRED_FIELDS.pop("resources", None)

RESOURCE_SPROUT_REQUIRED_FIELDS = checks.RESOURCE_REQUIRED_FIELDS | {
"title": RequiredFieldType.str,
"description": RequiredFieldType.str,
}
RESOURCE_SPROUT_REQUIRED_FIELDS.pop("data", None)
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
from pytest import mark

from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.checks.to_check_errors import (
from seedcase_sprout.core.checks.validation_errors_to_check_errors import (
COMPLEX_VALIDATORS,
to_check_errors,
validation_errors_to_check_errors,
)

simple_error = ValidationError(
Expand Down Expand Up @@ -52,7 +52,7 @@ def test_processes_flat_errors():
"""Should unwrap and transform a flat list of errors into `CheckError`s."""
errors = iter([simple_error])

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message=simple_error.message,
json_path=simple_error.json_path,
Expand All @@ -65,7 +65,7 @@ def test_processes_nested_errors():
"""Should flatten nested errors into a single list of `CheckError`s."""
errors = iter([nested_error])

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message=nested_error.context[0].message,
json_path="$.items[0].name",
Expand All @@ -83,7 +83,7 @@ def test_processes_deeply_nested_errors():
"""Should unwrap deeply nested errors and transform them into `CheckError`s."""
errors = iter([deeply_nested_error])

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message="Top-level error",
json_path="$.user",
Expand All @@ -109,7 +109,7 @@ def test_processes_multiple_toplevel_errors():
[simple_error, deeply_nested_error, simple_error, nested_error, simple_error]
)

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message=nested_error.context[0].message,
json_path="$.items[0].name",
Expand Down Expand Up @@ -156,14 +156,14 @@ def test_ignores_complex_validators(validator):
]
)

assert to_check_errors(errors) == []
assert validation_errors_to_check_errors(errors) == []


def test_removes_duplicate_errors():
"""Should remove duplicate `CheckError`s after transformation."""
errors = iter([simple_error] * 8)

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message=simple_error.message,
json_path=simple_error.json_path,
Expand All @@ -188,7 +188,7 @@ def test_sorts_errors_by_json_path():
]
)

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(message="Second error", json_path="$.data.a", validator="type"),
CheckError(message="First error", json_path="$.data.b", validator="type"),
CheckError(message="Third error", json_path="$.data.c", validator="type"),
Expand All @@ -205,7 +205,7 @@ def test_adds_field_name_for_required_errors():
]
)

assert to_check_errors(errors) == [
assert validation_errors_to_check_errors(errors) == [
CheckError(
message="'name' is a required property",
json_path="$.name",
Expand Down
4 changes: 2 additions & 2 deletions tests/core/sprout_checks/test_check_data_path_string.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ def test_passes_if_data_path_not_present(index):


@mark.parametrize("index", [None, 2])
def test_fails_if_path_not_string(index):
"""Should fail if the path is not of type string."""
def test_error_found_if_path_not_string(index):
"""Should find an error if the path is not of type string."""
properties = {"path": 123}

errors = check_data_path_string(properties, index)
Expand Down
4 changes: 2 additions & 2 deletions tests/core/sprout_checks/test_check_no_inline_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ def test_passes_if_data_not_set(index):


@mark.parametrize("index", [None, 2])
def test_fails_if_data_is_set(index):
"""Should fail if inline data is set."""
def test_error_found_if_data_is_set(index):
"""Should find an error if inline data is set."""
properties = {"data": "some data"}

errors = check_no_inline_data(properties, index)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ def test_passes_if_path_of_wrong_type(index, data_path):
Path("resources", "1", "data.parquet", "1"),
],
)
def test_fails_if_path_malformed(index, data_path):
"""Should fail if the path does not contain a resource ID."""
def test_returns_error_if_data_path_malformed(index, data_path):
martonvago marked this conversation as resolved.
Show resolved Hide resolved
"""Returns list of `CheckError`s if the data path does not contain a resource ID."""
properties = {"path": str(data_path)}

errors = check_resource_id_in_data_path(properties, index)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.sprout_checks.remove_not_sprout_related_resource_errors import ( # noqa: E501
remove_not_sprout_related_resource_errors,
from seedcase_sprout.core.sprout_checks.exclude_non_sprout_resource_errors import (
exclude_non_sprout_resource_errors,
)


def test_accepts_empty_list():
def test_returns_unaltered_empty_list():
"""Should not alter an empty list."""
assert remove_not_sprout_related_resource_errors([]) == []
assert exclude_non_sprout_resource_errors([]) == []


def test_removes_correct_errors():
def test_returns_only_sprout_related_errors():
"""Should only remove errors not relevant for Sprout."""
errors = [
CheckError(
Expand All @@ -35,7 +35,7 @@ def test_removes_correct_errors():
),
]

assert remove_not_sprout_related_resource_errors(errors) == [
assert exclude_non_sprout_resource_errors(errors) == [
CheckError(
message="'name' is a required property",
json_path="$.name",
Expand Down
4 changes: 2 additions & 2 deletions tests/core/sprout_checks/test_get_blank_value_for_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@
("something else", None),
],
)
def test_returns_correct_value(type, value):
"""Should return the correct blank value for each type."""
def test_returns_expected_blank_value_for_each_type(type, value):
"""Should return the expected blank value for each type."""
assert get_blank_value_for_type(type) == value
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
)


def test_returns_correct_path_without_index():
def test_returns_expected_json_path_without_index():
"""Should form the correct JSON path with no index supplied."""
assert get_json_path_to_resource_field("myField") == "$.myField"

Expand Down