feat: ✨ add `RequiredCheck` #122

martonvago · 2025-10-13T13:57:26Z

Description

This PR handles the special case of requiring a field to be non-null by subclassing CustomCheck to create RequiredCheck.

Usage:

version_required = RequiredCheck(
    jsonpath="$.version",
    message="Version is required.",
)
lowercase_check = CustomCheck(
    jsonpath="$.name",
    message="Name must be lowercase.",
    check=lambda name: name.islower(),
    type="lowercase",
)
config = Config(custom_checks=[lowercase_check, version_required])
issues = check(descriptor, config=config)

I thought this was a nice way of splitting out the logic for required rules and that required rules were special enough to warrant their own class.

I also thought it made sense to limit what kind of JSON paths the required rule can be applied to. It makes sense to mark a dict property (e.g. $.resources[*].title) as required because it is very obvious what it means for this field to be missing.

But marking an array item itself (e.g. $.resources[*] or $.resources[2]) as required makes less sense to me. "I want the second resource to exist in particular" is a weird constraint. If someone cares about the number of resources, they should be checking the length of the array.
Same for "vague" JSON paths like $..title: selecting all title fields under the root node and then checking if they exist feels circular and unnecessary.

➡️ So I restricted RequiredCheck to sensible paths only, which also made the code simpler.

Closes #120

Needs an in-depth review.

Checklist

Formatted Markdown
Ran just run-all

martonvago · 2025-10-14T11:29:30Z

src/check_datapackage/internals.py

+def _get_direct_jsonpaths(jsonpath: str, descriptor: dict[str, Any]) -> list[str]:
+    """Returns all direct JSON paths that match a direct or indirect JSON path."""
+    fields = _get_fields_at_jsonpath(jsonpath, descriptor)
+    return _map(fields, lambda field: field.jsonpath)


Exclude can use this too, that's why I put it here

martonvago · 2025-10-14T11:31:15Z

src/check_datapackage/custom_check.py

-    """Checks the descriptor against the rule and creates issues for fields that fail.
+    def __init__(self, jsonpath: str, message: str):
+        """Initializes the `RequiredRule`."""
+        field_name_match = re.search(r"(?<!\.)(\.\w+)$", jsonpath)


Find field names not preceded by .. So, e.g., $.resources[*].name >> .name

martonvago · 2025-10-14T11:32:34Z

src/check_datapackage/custom_check.py

+                f"Cannot define `RequiredRule` for JSON path `{jsonpath}`."
+                " A `RequiredRule` must target a concrete object field (e.g.,"
+                " `$.title`) or set of fields (e.g., `$.resources[*].title`)."
+                " Ambiguous paths (e.g., `$..title`) or paths pointing to array items"


Better alternatives for "ambiguous"?

martonvago · 2025-10-14T11:38:21Z

src/check_datapackage/custom_check.py

+        matching_paths = _get_direct_jsonpaths(self.jsonpath, descriptor)
+        indirect_parent_path = self.jsonpath.removesuffix(self._field_name)
+        direct_parent_paths = _get_direct_jsonpaths(indirect_parent_path, descriptor)
+        missing_paths = _filter(
+            direct_parent_paths,
+            lambda path: f"{path}{self._field_name}" not in matching_paths,
+        )


Conceptually:

Get all existing paths >> ["$.resources[0].name", "$.resources[1].name"]

Get all parent paths >> ["$.resources[0]", "$.resources[1]", "$.resources[2]"]

Get all missing paths >> ["$.resources[2].name"]

martonvago · 2025-10-14T11:40:40Z

src/check_datapackage/custom_check.py

@@ -43,38 +45,105 @@ class Rule:
    check: Callable[[Any], bool]
    type: str = "custom"

+    def apply(self, descriptor: dict[str, Any]) -> list[Issue]:


Made apply a method on the class. This way, Rules and RequiredRules can each define their own logic.

martonvago · 2025-10-14T11:44:45Z

src/check_datapackage/rule.py

    _get_fields_at_jsonpath,
    _map,
 )
 from check_datapackage.issue import Issue


-@dataclass
+@dataclass(frozen=True)
 class Rule:


A weakness here is that there is nothing stopping you from writing Rule(type="required", ...) and this will not do anything, just like before. There are some ways of disallowing this, but maybe it's better not to complicate it further. The guide can say "if you want to check that a property exists, use the RequiredRule".

What do you think?

lwjohnst86

Some initial thoughts before I do a deeper review ☺️ Very neat though!

lwjohnst86 · 2025-10-17T13:39:11Z

src/check_datapackage/__init__.py

    "Exclude",
    "Issue",
    "CustomCheck",
+    "RequiredCheck",


You're exposing it, but how it is integrated into Config?

There's an example in the description of the PR. No need to do anything special because a RequiredCheck is a CustomCheck ☺️

lwjohnst86 · 2025-10-17T13:44:18Z

src/check_datapackage/custom_check.py

+        )
+
+
+class RequiredCheck(CustomCheck):


Not sure I like that it's a whole other exposed class that we have to describe and document. Plus the naming now doesn't match the style of CustomCheck. Do you think we could fold in having a required check into the existing exposed CustomCheck?

I think I can fold all the code into CustomCheck. I could have some if type == "required" conditionals and hope it doesn't become too spaghetti?

But then how would you want to handle the creation of a CustomCheck with type="required"? Should it be possible for the user to pass their own check function? What if they define something other than the non-null condition?
We could make the required check the default custom check maybe?

martonvago · 2025-10-20T15:26:56Z

I opened an alternative (PR #146), but I'll leave this here so you can compare ☺️

feat: ✨ add RequiredRule

20ced70

martonvago self-assigned this Oct 13, 2025

martonvago added this to Iteration planning Oct 13, 2025

github-project-automation bot moved this to Todo in Iteration planning Oct 13, 2025

martonvago moved this from Todo to In Progress in Iteration planning Oct 13, 2025

martonvago added 4 commits October 14, 2025 10:15

refactor: ♻️ simplify apply logic

520f04a

fix: 🐛 remove unused import

3b8a7c3

fix: 🐛 disallow all ambiguous JSON paths

e0f3ac9

Merge branch 'main' into feat/required-rule

7a1b03d

martonvago commented Oct 14, 2025

View reviewed changes

martonvago moved this from In Progress to In Review in Iteration planning Oct 14, 2025

martonvago marked this pull request as ready for review October 14, 2025 12:05

martonvago requested a review from a team as a code owner October 14, 2025 12:05

martonvago added 3 commits October 15, 2025 08:54

Merge branch 'main' into feat/required-rule

4ecfd81

Merge branch 'main' into feat/required-rule

4a729b5

Merge branch 'main' into feat/required-rule

11d80d4

lwjohnst86 requested changes Oct 17, 2025

View reviewed changes

github-project-automation bot moved this from In Review to In Progress in Iteration planning Oct 17, 2025

signekb changed the title ~~feat: ✨ add RequiredRule~~ feat: ✨ add RequiredRule Oct 17, 2025

martonvago changed the title ~~feat: ✨ add RequiredRule~~ feat: ✨ add RequiredCheck Oct 17, 2025

martonvago mentioned this pull request Oct 20, 2025

feat: ✨ add check_missing to CustomCheck #146

Open

2 tasks

martonvago moved this from In Progress to In Review in Iteration planning Oct 20, 2025

martonvago mentioned this pull request Oct 21, 2025

Rules with type="required" #120

Open

feat: ✨ add RequiredCheck #122

Are you sure you want to change the base?

feat: ✨ add RequiredCheck #122

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

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!

lwjohnst86 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!

martonvago Oct 17, 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!

martonvago commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat: ✨ add `RequiredCheck` #122

feat: ✨ add `RequiredCheck` #122

martonvago commented Oct 13, 2025 •

edited

Loading

martonvago Oct 17, 2025 •

edited

Loading