Deprecate Message method get_combined_intent_response_key #9088
Conversation
kedz
requested review from
a team and
koernerfelicia
and removed request for
a team and
koernerfelicia
| @staticmethod | ||
| def separate_intent_response_key( | ||
| original_intent: Text, | ||
| ) -> Tuple[Text, Optional[Text]]: | ||
|
|
||
| """Split intent into main intent name and optional sub-intent name.""" |
There was a problem hiding this comment.
| """Split intent into main intent name and optional sub-intent name.""" | |
| """Splits intent into main intent name and optional sub-intent name.""" |
There was a problem hiding this comment.
should we give a concrete example?
There was a problem hiding this comment.
+1 (though I see now I was removed as reviewer :) )
There was a problem hiding this comment.
@joejuzl adding you since Tobias in on vacation. So I was thinking of adding this example:
"""Splits intent into main intent name and optional sub-intent name.
For example, `"FAQ/how_to_contribute"` would be split into
`("FAQ", "how_to_contribute")`. If there is no response delimiter, the
second tuple item is `None`, e.g. "FAQ" would be mapped to `("FAQ", None)`.
"""My only hesitation with this is that we don't actually split on "/" but rather the constant RESPONSE_IDENTIFIER_DELIMITER. So there is the risk of this docstring going stale if that delimiter ever changes.
I think this alternative while accurate is somewhat confusing:
"""Splits intent into main intent name and optional sub-intent name.
For example, `f"FAQ{RESPONSE_IDENTIFIER_DELIMITER}how_to_contribute"` would be split into
`("FAQ", "how_to_contribute")`. If there is no response delimiter, the
second tuple item is `None`, e.g. "FAQ" would be mapped to `("FAQ", None)`.
"""Do you think the first is ok to use or do you have an alternative suggestion?
There was a problem hiding this comment.
Does it make sense to do both?
"""Splits intent into main intent name and optional sub-intent name.
For example, `"FAQ/how_to_contribute"` would be split into
`("FAQ", "how_to_contribute")`. The response delimiter can
take different values (not just "/") and depends on the
constant - RESPONSE_IDENTIFIER_DELIMITER.
If there is no response delimiter, the second tuple item
is `None`, e.g. "FAQ" would be mapped to `("FAQ", None)`.
"""There was a problem hiding this comment.
@jupyterjazz yeah I like that. Will make that change.
There was a problem hiding this comment.
Sorry just saw this. We've had similar discussion in the past about how to handle constants in docstrings - it's a tricky one :)
The only problem I have with the proposed solution is that this is a docstring for a public method, and RESPONSE_IDENTIFIER_DELIMITER is not really a configurable thing, so it might be slightly confusing.
Is it not possible to fstring the value into the docstrings? That way it would always be up-to-date with the constant.
Co-authored-by: Tobias Wochinger <[email protected]>
Proposed changes:
get_combined_intent_response_keymethod ofMessageclass fromrasa.shared.nlu.training_data.message.py.Messagemethod #8883Status (please check what you already did):
[ ] added some tests for the functionality[ ] updated the documentationblack(please check Readme for instructions)