Skip to content
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

Issue 3819 - pass message_id to _rasa_http_parse #3822

Merged
merged 11 commits into from
Jul 9, 2019
Merged

Issue 3819 - pass message_id to _rasa_http_parse #3822

merged 11 commits into from
Jul 9, 2019

Conversation

EdouM
Copy link
Contributor

@EdouM EdouM commented Jun 19, 2019

Proposed changes:

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality (updated the current test actually)
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2019

CLA assistant check
All committers have signed the CLA.

@akelad
Copy link
Contributor

akelad commented Jun 19, 2019

Thanks for submitting this PR, we'll give it a review as soon as possible

@tmbo tmbo requested a review from tabergma June 25, 2019 12:39
@tabergma
Copy link
Contributor

@EdouM Implementation looks good. Just a quick question: Does it also make sense to add it the /model/parse endpoint of Rasa (see https://github.com/RasaHQ/rasa/blob/master/rasa/server.py#L802)?

@EdouM
Copy link
Contributor Author

EdouM commented Jul 1, 2019

@tabergma indeed it makes sense to add message_id to the /model/parse endpoint. I committed the change, let me know if anything else is needed!

Copy link
Contributor

@tabergma tabergma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far, but you need to add two more things:
First, please update the API specs: https://github.com/RasaHQ/rasa/blob/master/docs/_static/spec/rasa.yml#L530
Second, you need to pass on the message_id here: https://github.com/RasaHQ/rasa/blob/master/rasa/core/interpreter.py#L258

@EdouM
Copy link
Contributor Author

EdouM commented Jul 8, 2019

@tabergma thanks for the suggestions, I just reviewed and pushed them. Sorry for the back and forth I should have realized this on my own but I appreciate the feedback.

Copy link
Contributor

@tabergma tabergma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks a lot for adding the remaining things 🚀 Will merge once the tests pass.

@tabergma tabergma merged commit e649e5f into RasaHQ:master Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message_id not passed in _rasa_http_parse in RasaNLUHttpInterpreter
4 participants