-
Notifications
You must be signed in to change notification settings - Fork 233
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
Return 449 status code if the Domain is not stored or missing in the … #1104
Return 449 status code if the Domain is not stored or missing in the … #1104
Conversation
rasa_sdk/executor.py
Outdated
@@ -162,6 +166,8 @@ def __init__(self) -> None: | |||
self.actions: Dict[Text, Callable] = {} | |||
self._modules: Dict[Text, TimestampModule] = {} | |||
self._loaded: Set[Type[Action]] = set() | |||
self.domain: Dict[Text, Any] = {} |
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.
Have you considered to use None
instead of empty dict and empty string below?
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 thought it was more safe to have these variables be of the type it is expected. It's also how variables above are defined. You want to differentiate a case where if it's None
at first it means that there is no Domain information, and if it is {}
and ""
later, the Domain information is there, it's just empty Domain, right?
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.
Yea, I was thinking about clearly differentiating case when there is a domain (whether it may be empty or not) and when there is no domain info at all.
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.
Noted, makes sense to me, I'll update and test it.
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 had to update the typing to use Optional[...]
so that None
value can be accepted. This resulted in additional changes so that the code can pass the Code Quality check, like removing ActionCall
as typing and using Dict[Text, Any]
(which is the correct type for action_call
.
rasa_sdk/executor.py
Outdated
""" | ||
return bool(self.domain_digest) and self.domain_digest == domain_digest | ||
|
||
def get_domain( |
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.
Word get
suggests that the method is not doing any mutation of the data while in fact it is. It is mutating the state of the parent object.
I would suggest to explore different name like: upsert_domain
or update_domain
etc.
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.
You are right. Though this method is not just "upserting" the Domain, it's also returning the result. Should the naming match that behavior as well? What would you name it in that case?
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 updated the method name to update_and_return_domain
to clearly state that we are making changes to the domain fields of the parent object AND also returning the domain.
Also updated the docstring to describe this in a more precise manner.
Spike got approved, closing this PR, will be used as the inspiration for the implementation. |
Proposed changes:
Status (please check what you already did):
black
(please check Readme for instructions)