Skip to content
This repository has been archived by the owner on Aug 22, 2019. It is now read-only.

WIP: sort the scores array in server.py #1733

Merged
merged 7 commits into from
Feb 25, 2019

Conversation

therealdeadly
Copy link
Contributor

@therealdeadly therealdeadly commented Feb 19, 2019

#1707
Proposed changes:

  • Sorting the actions in score array returned from HTTP API endpoint :- /conversations/{sender_id}/predict
  • Remove the sorting logic from training.interactive._request_action_from_user()

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

@tmbo I have added-deleted the sort from the aforementioned places, what do you think? Also, I am confused about writing new tests for these changes, the tests already in test_server.py should suffice right? or am I missing something?

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2019

CLA assistant check
All committers have signed the CLA.

@@ -407,6 +407,9 @@ def predict(sender_id):
try:
# Fetches the appropriate bot response in a json format
responses = agent.predict_next(sender_id)
responses['scores'] = sorted(responses['scores'],
Copy link
Member

Choose a reason for hiding this comment

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

would you mind adding a test for this as well? (also would be great if you can update the api documentation as well as the CHANGELOG.rst)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have added a test and updated the api documentation and CHANGELOG.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmbo hey, can you please review the additions?

@codeclimate
Copy link

codeclimate bot commented Feb 22, 2019

Code Climate has analyzed commit e40a41a and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@tmbo tmbo 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, thanks a lot for the change 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants