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

Merged
merged 33 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 29 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
29 changes: 29 additions & 0 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
@@ -0,0 +1,29 @@
from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


def check_data_path_string(
properties: dict, index: int | None = None
) -> list[CheckError]:
"""Checks that the `path` field of a set of resource properties is of type string.

Args:
properties: The resource properties.
index: The index of the resource properties. Defaults to None.

Returns:
A list of errors. An empty list if no error was found.
"""
path = properties.get("path", "")
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",
)
]
31 changes: 31 additions & 0 deletions seedcase_sprout/core/sprout_checks/check_no_inline_data.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


def check_no_inline_data(
properties: dict, index: int | None = None
) -> list[CheckError]:
signekb marked this conversation as resolved.
Show resolved Hide resolved
"""Checks that the `data` field of a set of resource properties is not set.

Args:
properties: The resource properties.
index: The index of the resource properties. Defaults to None.

Returns:
A list of errors. The empty list if no error was found.
"""
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
@@ -0,0 +1,37 @@
from pathlib import Path

from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


def check_resource_id_in_data_path(
martonvago marked this conversation as resolved.
Show resolved Hide resolved
properties: dict, index: int | None = None
) -> list[CheckError]:
"""Checks if the data path in resource properties is well-formed.

Ignores a missing data path or a path of the wrong type.

Args:
properties: The resource properties to check.
index: The index of the resource properties. Defaults to None.

Returns:
The properties, if the data path is well-formed.
"""
data_path = properties.get("path")
if not isinstance(data_path, str):
return []

data_path = Path(data_path)
if len(data_path.parts) == 3 and data_path.parts[1].isdigit():
return []

return [
CheckError(
message="'path' should contain the resource ID",
lwjohnst86 marked this conversation as resolved.
Show resolved Hide resolved
json_path=get_json_path_to_resource_field("path", index),
validator="pattern",
)
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from seedcase_sprout.core.checks.check_error import CheckError


def exclude_non_sprout_resource_errors(
errors: list[CheckError],
) -> list[CheckError]:
"""Filters out resource errors that are not relevant for Sprout.

Errors filtered out:
- inline `data` required but missing
- `path` is not of type array

Args:
errors: The full error list.

Returns:
The filtered error list.
"""
return [
error
for error in errors
if not (error.validator == "required" and error.json_path.endswith(".data"))
and not (
error.validator == "type"
and error.json_path.endswith(".path")
and error.message.endswith("not of type 'array'")
)
]
19 changes: 19 additions & 0 deletions seedcase_sprout/core/sprout_checks/get_blank_value_for_type.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from seedcase_sprout.core.checks.required_fields import RequiredFieldType


def get_blank_value_for_type(type: RequiredFieldType) -> str | list | None:
"""Returns the blank value for each type of (required) field.
signekb marked this conversation as resolved.
Show resolved Hide resolved

Args:
type: The type of the field.

Returns:
The corresponding blank value.
"""
match type:
case RequiredFieldType.str:
return ""
case RequiredFieldType.list:
return []
case _:
return None
signekb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
def get_json_path_to_resource_field(field: str, index: int | None = None) -> str:
"""Creates the JSON path to the specified field of a set of resource properties.

Optionally adds the index of the resource properties, if they are part of a set of
package properties.

Args:
field: The name of the field.
index: The index of the resource properties. Defaults to None.

Returns:
The JSON path.
"""
return "$." + ("" if index is None else f"resources[{index}].") + field
35 changes: 35 additions & 0 deletions tests/core/sprout_checks/test_check_data_path_string.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
from pytest import mark

from seedcase_sprout.core.sprout_checks.check_data_path_string import (
check_data_path_string,
)
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


@mark.parametrize("index", [None, 2])
def test_passes_if_data_path_string(index):
"""Should pass if the path is of type string."""
properties = {"path": "a string"}

assert check_data_path_string(properties, index) == []


@mark.parametrize("index", [None, 2])
def test_passes_if_data_path_not_present(index):
"""Should pass if the path is not set."""
assert check_data_path_string({}, index) == []


@mark.parametrize("index", [None, 2])
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)

assert len(errors) == 1
assert errors[0].message == "123 is not of type 'string'"
martonvago marked this conversation as resolved.
Show resolved Hide resolved
assert errors[0].json_path == get_json_path_to_resource_field("path", index)
assert errors[0].validator == "type"
25 changes: 25 additions & 0 deletions tests/core/sprout_checks/test_check_no_inline_data.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from pytest import mark

from seedcase_sprout.core.sprout_checks.check_no_inline_data import check_no_inline_data
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


@mark.parametrize("index", [None, 2])
def test_passes_if_data_not_set(index):
"""Should pass if inline data is not set."""
assert check_no_inline_data({}, index) == []


@mark.parametrize("index", [None, 2])
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)

assert len(errors) == 1
assert "'data' should not be set" in errors[0].message
lwjohnst86 marked this conversation as resolved.
Show resolved Hide resolved
assert errors[0].json_path == get_json_path_to_resource_field("data", index)
assert errors[0].validator == "inline-data"
55 changes: 55 additions & 0 deletions tests/core/sprout_checks/test_check_resource_id_in_data_path.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
from pathlib import Path

from pytest import mark

from seedcase_sprout.core.sprout_checks.check_resource_id_in_data_path import (
check_resource_id_in_data_path,
)
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


@mark.parametrize("index", [None, 2])
def test_passes_if_data_path_well_formed(index):
"""Should pass if the path contains a resource ID."""
properties = {"path": str(Path("resources", "1", "data.parquet"))}

assert check_resource_id_in_data_path(properties, index) == []


@mark.parametrize("index", [None, 2])
def test_passes_if_data_path_not_present(index):
"""Should pass if the path is not set."""
assert check_resource_id_in_data_path({}, index) == []


@mark.parametrize("index", [None, 2])
@mark.parametrize("data_path", [123, []])
def test_passes_if_path_of_wrong_type(index, data_path):
"""Should pass if path is of the wrong type."""
properties = {"path": data_path}

assert check_resource_id_in_data_path(properties, index) == []


@mark.parametrize("index", [None, 2])
@mark.parametrize(
"data_path",
[
"",
Path("resources", "x", "data.parquet"),
Path("1", "data.parquet"),
Path("resources", "1", "data.parquet", "1"),
],
)
def test_returns_error_if_data_path_is_malformed(index, data_path):
"""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)

assert len(errors) == 1
assert errors[0].message == "'path' should contain the resource ID"
assert errors[0].json_path == get_json_path_to_resource_field("path", index)
assert errors[0].validator == "pattern"
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.sprout_checks.exclude_non_sprout_resource_errors import (
exclude_non_sprout_resource_errors,
)


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


def test_returns_only_sprout_related_errors():
"""Should only remove errors not relevant for Sprout."""
errors = [
CheckError(
message="'data' is a required property",
json_path="$.data",
validator="required",
),
CheckError(
message="'name' is a required property",
json_path="$.name",
validator="required",
),
CheckError(
message="123 is not of type 'array'", json_path="$.path", validator="type"
),
CheckError(
message="123 is not of type 'string'", json_path="$.path", validator="type"
),
CheckError(
message="123 is not of type 'array'",
json_path="$.sources",
validator="type",
),
]

assert exclude_non_sprout_resource_errors(errors) == [
CheckError(
message="'name' is a required property",
json_path="$.name",
validator="required",
),
CheckError(
message="123 is not of type 'string'", json_path="$.path", validator="type"
),
CheckError(
message="123 is not of type 'array'",
json_path="$.sources",
validator="type",
),
]
21 changes: 21 additions & 0 deletions tests/core/sprout_checks/test_get_blank_value_for_type.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from pytest import mark

from seedcase_sprout.core.checks.required_fields import RequiredFieldType
from seedcase_sprout.core.sprout_checks.get_blank_value_for_type import (
get_blank_value_for_type,
)


@mark.parametrize(
"type,value",
[
(RequiredFieldType.str, ""),
(RequiredFieldType.list, []),
("int", None),
(None, None),
("something else", None),
],
)
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
@@ -0,0 +1,13 @@
from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import (
get_json_path_to_resource_field,
)


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"


def test_returns_correct_path_with_index():
"""Should form the correct JSON path with a resource index supplied."""
assert get_json_path_to_resource_field("myField", 2) == "$.resources[2].myField"
Loading