-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add error message for rasa validate when text is null in a response #8618
Conversation
rasa/shared/utils/validation.py
Outdated
@@ -168,12 +168,17 @@ def validate_yaml_schema(yaml_file_content: Text, schema_path: Text) -> None: | |||
|
|||
try: | |||
c.validate(raise_exception=True) | |||
except SchemaError: | |||
except SchemaError as e: | |||
if e not in c.errors: |
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.
Are these the same kind of errors? Are they even comparable?
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 couldn't find much about SchemaError in pykwalify
documentation, but in their library they're the same kind (c.errors
is a list of SchemaErrors)
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 double-checked and you're right @joejuzl , there is a nuance, c.errors
is a list of SchemaError.SchemaErrorEntry
which is an inner class.
I can check e.msg
against each message in c.errors
to prevent duplication but it doesn't seem to cause any issues in our code if I add the SchemaError
in the list of validation_errors alongside c.errors
🤔
I might have to change the type annotation to Union of SchemaError and SchemaErrorEntry in the YamlValidationException
. WDYT?
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 think it was fine how it was to begin with tbh. As we know what they are not duplicate now.
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've actually kept the check at msg
level because in some cases there might be duplicates based on what I could understand from pykwalify
codebase - for example when the SchemaError
that gets raised during validation includes the c.errors
messages rather than it being a custom SchemaError that Rasa introduces through pykwalify extensions. I'm going to have to edit the check (reverse it) based on this insight.
Ok so I looked a bit deeper into the pykwalify code... and I think the problem is with our extension. Because we |
@joejuzl Yup, that's what I understood as well, but didn't know you can return a Error 😄 so try changing L20 in |
I actually just returned a string value, but does returning the SchemaError also work? |
@joejuzl yup it works with both string and SchemaError - the printed error message will differ just slightly:
Which one do you think it's more informative for the user? I'd vote for 2nd since it makes it clear it's a schema related error, however it's not nicely formatted and that might confuse users? |
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 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.
🚀🚀🚀
…ema error message
644d938
to
185f7e7
Compare
Proposed changes:
Status (please check what you already did):
black
(please check Readme for instructions)