Allow passing dynamic timezone to duckling ISSUE-2801 #10781
Conversation
|
Anthony seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
AnthonyDurussel
changed the title
|
Hey @wochinge, would be awesome if you could merge this! Please let us know what is missing so we can work on it. It's a feature that is much needed for duckling and hasn't been implemented (while the issue is quite old...). Thanks |
There was a problem hiding this comment.
Thanks for the PR! I left some comments on the PR itself, but in general I'm not clear why the addition of explicit metadata is necessary, since it's already available in NLU components. Could you clarify your use case and also add tests for the presence and absence of this metadata key?
| matches = self._duckling_parse(message.get(TEXT), reference_time) | ||
| matches = self._duckling_parse( | ||
| message.get(TEXT), | ||
| message.get(METADATA).get(METADATA_TIME_ZONE), |
There was a problem hiding this comment.
This could throw an error in the absence of metadata
| parsed_data = await app.agent.parse_message( | ||
| data.get("text"), data.get("metadata") | ||
| ) |
There was a problem hiding this comment.
The metadata should already form part of the standard UserMessage object that is passed all the way down to Duckling. Can you clarify why it's necessary to pass it in explicitly here?
| _metadata = {} | ||
| for k, v in data.items(): | ||
| if k not in ("text", "model", "time"): | ||
| _metadata[k] = v | ||
| if _metadata: | ||
| _data["metadata"] = _metadata |
There was a problem hiding this comment.
I'm not sure why this is needed?
Proposed changes:
Issue: #2801
time_zoneproperty.Status (please check what you already did):
black(please check Readme for instructions)