-
Notifications
You must be signed in to change notification settings - Fork 0
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 CheckErrorMatcher
object
#972
Conversation
Co-authored-by: Luke W. Johnston <[email protected]>
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
…checks-2-simple-helper-functions
…/sprout-checks-3-required-and-blank-checks
…at/sprout-checks-4-sprout-specific-package-and-resource-errors
…ce-errors' into feat/sprout-checks-5-check-resource-properties
Co-authored-by: Luke W. Johnston <[email protected]>
…at/sprout-checks-6-check-package-properties
Co-authored-by: Luke W. Johnston <[email protected]>
…t/sprout-checks-7-check-properties
Co-authored-by: Luke W. Johnston <[email protected]>
…eck-error-matcher
CheckErrorMatcher
object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time seeing what this does or it's usefulness, but I think it would help once we move these functionality over into a standalone package with some guides showing the functions in action. Or seeing the functions in action in other functions within Sprout. But you're very much knowledgeable about what's needed and the challenges in this check area, so 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍 Just a suggestion and a question to help me understand 🔍
"json_path,matcher,expected", | ||
[ | ||
("$.match", None, True), | ||
("$.no.match", "", False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a very basic answer to this but why do we want the match to be false here, while the message is matched (True) when the matcher is an empty string (L12)?
Same with the validator below (L61)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very solid reasons 😅! I was thinking:
message_matches
should check if the matcher message is a substring of the error message. Doesn't matter that much if""
matches or not?json_path_matches
should check if the matcher json path is exactly the full path or exactly the very last field.""
is neither, so I made it not match anything.validator_matches
should check if the matcher validator is exactly the validator of the error.""
is not a validator, so I made it not match.
We can refine the behaviour further for sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think that makes sense! Thanks for elaborating 👍
Description
This PR adds a
CheckErrorMatcher
object. This is/will be used to filter out unwanted errors easily.Example usage:
I'm going to use this to replace the
check_required
flag in Sprout's check functions.This PR needs an in-depth review.
Checklist
just run-all