-
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
Raising exception when having empty intent or entity mapping in the domain #8222
Conversation
0f2dff7
to
f90ac7f
Compare
- Replaced try/except statement with if/else when checking if intent mapping is empty. - Minor rewording on the exception message. - Added a new test to check if the exception is correctly raised.
f90ac7f
to
be336d9
Compare
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.
Approved but please take a look at the two small comments I've left :)
- Rewording of the function's docstring. - Rewording of the InvalidDomain exception message.
Hi @alwx ! Thank you so much for your review and constructive feedback! Please let me know if any further rewording is needed in the docstring or the exception. After making the current changes for the empty intent mapping, I also thought that a user might also insert an empty entity mapping so in the
instead of the correct one :
and the error we get when having the empty entity mapping is the following :
I checked the code and saw that if I add a check here, then instead of the error I can raise another InvalidDomain exception message. |
Since it's a small change and because it's related to what you've done here, then I think it's perfectly fine to do it in the same PR. Don't forget to update the changelog entry in this case! :) |
- Added a check to handle also empty entity mapping. - Changed the changelog. - Added a test for the empty entity mapping.
@joejuzl Do you have time for another quick review so this PR can get wrapped up? |
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.
LGTM
Proposed changes:
domain.yml
file.Status (please check what you already did):
black
(please check Readme for instructions)