feat: ✨ add RequiredCheck
#122
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martonvago
commented
Oct 14, 2025
martonvago
commented
Oct 14, 2025
| """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) |
Contributor
Author
There was a problem hiding this comment.
Find field names not preceded by .. So, e.g., $.resources[*].name >> .name
martonvago
commented
Oct 14, 2025
martonvago
commented
Oct 14, 2025
martonvago
commented
Oct 14, 2025
martonvago
commented
Oct 14, 2025
lwjohnst86
requested changes
Oct 17, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Some initial thoughts before I do a deeper review
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
2 tasks
Contributor
Author
|
I opened an alternative (PR #146), but I'll leave this here so you can compare |
2 tasks
…k-datapackage into feat/required-rule
martonvago
commented
Oct 28, 2025
martonvago
commented
Oct 28, 2025
martonvago
commented
Oct 28, 2025
lwjohnst86
requested changes
Oct 28, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Nice progress! Some more comments 😁
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
martonvago
commented
Oct 29, 2025
Comment on lines
+110
to
+111
| if not field_name_match: | ||
| return [] |
Contributor
Author
There was a problem hiding this comment.
This amounts to silently ignoring incompatible JSON paths for now.
lwjohnst86
approved these changes
Oct 29, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Supperrrr nice!! 🎉 Great job 😁 🦾
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
github-project-automation
bot
moved this from In Progress
to Done
in Iteration planning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR handles the special case of requiring a field to be non-null by subclassing
CustomCheckto createRequiredCheck.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.$.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.$..title: selecting alltitlefields under the root node and then checking if they exist feels circular and unnecessary.➡️ So I restricted
RequiredCheckto sensible paths only, which also made the code simpler.Closes #120
Needs an in-depth review.
Checklist
just run-all