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

Added template for image, attachment, quick_replies, buttons #4077

Merged
merged 22 commits into from Jul 25, 2019
Merged

Added template for image, attachment, quick_replies, buttons #4077

merged 22 commits into from Jul 25, 2019

Conversation

RanaMostafaAbdElMohsen
Copy link
Contributor

@RanaMostafaAbdElMohsen RanaMostafaAbdElMohsen commented Jul 22, 2019

Fixes for #3763:

  • Supporting fill slots in other part of utterance templates other than text

Status (please check what you already did):

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

@msamogh
Copy link
Contributor

msamogh commented Jul 23, 2019

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

@msamogh msamogh requested a review from wochinge July 23, 2019 06:14
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for your cool addition 🎉 Great to have such motivated community members 💯
I left a few comments :-) Could you please also update the changelog?

rasa/core/nlg/template.py Outdated Show resolved Hide resolved
tests/core/test_nlg.py Show resolved Hide resolved
@wochinge
Copy link
Contributor

@RanaMostafaAbdElMohsen Just the changelog missing :-)

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

@RanaMostafaAbdElMohsen Just the changelog missing :-)
Yes sure 👍

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 23, 2019

@RanaMostafaAbdElMohsen Just the changelog missing :-)
We should update log in this section [Unreleased 1.1.8] correct?

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

@wochinge, Please check if there are any additional comments

CHANGELOG.rst Outdated Show resolved Hide resolved
tests/core/test_nlg.py Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor

@RanaMostafaAbdElMohsen added two more comments :-)

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

@wochinge , please check again. Done :)

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

One more suggestion regarding the changelog and there are a couple of merge conflicts with master. Tests are looking great 🎸

CHANGELOG.rst Outdated Show resolved Hide resolved
@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 24, 2019

One more suggestion regarding the changelog and there are a couple of merge conflicts with master. Tests are looking great 🎸

Done @wochinge. Can you please advise why the coverage decreased, although I covered all the test cases ?

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

sorry, there were so many changes in this module in the mean while 🙈
added another comment. Sorry, for the hassle!
Could you also remove the unused imports please?

rasa/core/nlg/template.py Outdated Show resolved Hide resolved
@RanaMostafaAbdElMohsen RanaMostafaAbdElMohsen changed the title Added template for image Added template for image, attachment, quick_replies, buttons Jul 24, 2019
@RanaMostafaAbdElMohsen
Copy link
Contributor Author

I will fill slots for buttons, attachments, quick_replies as well as images and will update CHANGELOG.rst @wochinge

@wochinge
Copy link
Contributor

@RanaMostafaAbdElMohsen Are you sure it's actually a use case to have that in buttons, attachments, and quick_replies? We can also do images first and then see whether others actually need interpolation for the others.

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 24, 2019

@RanaMostafaAbdElMohsen Are you sure it's actually a use case to have that in buttons, attachments, and quick_replies? We can also do images first and then see whether others actually need interpolation for the others.

@wochinge
As referred to discussion here #3763, it seems it is needed to support those templates as well

@wochinge
Copy link
Contributor

@RanaMostafaAbdElMohsen Awesome, then let's update the changelog and merge this :-)

CHANGELOG.rst Outdated Show resolved Hide resolved
Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>
@RanaMostafaAbdElMohsen
Copy link
Contributor Author

Done @wochinge

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

awesome thanks for your contribution 🥇

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.

None yet

3 participants