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.

Sign up for GitHub

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

Jump to bottom

Currency feature #18

Merged
merged 6 commits into from
Apr 27, 2016
Merged

Currency feature #18

merged 6 commits into from
Apr 27, 2016

Conversation

havanagrawal
Copy link
Contributor

Using the API from fixer.io

Updated intents and entities on https://wit.ai/havanagrawal/JARVIS-on-Messenger

@swapagarwal
Copy link
Owner

swapagarwal commented Apr 23, 2016 •
edited
Loading

Whoa, you sure are fast! 😮
The API looks nice, however wouldn't it be better if we could support conversion instead of just supplying the rate? (like 100 USD to INR)
And your HKD to USD test is failing, which I suppose is because HKD was encountered for the first time? So the user has to remember and query for these specific inputs instead of natural query involving dollars, pounds, rupees, etc. Can you search for a better API? If there does not exist one, then we can go forward without it.

@havanagrawal
Copy link
Contributor Author

Thanks 😄
I think the test is failing because I tested locally with my own Wit access token, but didn't commit the token. So it tries to run the test, but with the original token, it has no clue about "currency" as an intent (https://wit.ai/swapagarwal/JARVIS-on-Messenger/eval?_t=83&q=convert+USD+to+HKD)?

Could you please try adding the intent "currency", and the entities "from_currency" and "to_currency", like I've set up on https://wit.ai/havanagrawal/JARVIS-on-Messenger ? If the test still fails, I'll make it more robust.

@havanagrawal
Copy link
Contributor Author

Also, I'll try to get Wit to recognize the amount along with the currencies, i.e. completely support conversion.

@swapagarwal
Copy link
Owner

swapagarwal commented Apr 23, 2016 •
edited
Loading

Ofcourse, my bad! I'll set it up and restart the build. Also, why have you set them up as traits?
Take a look at the wit/amount_of_money entity for currency conversion.

@havanagrawal
Copy link
Contributor Author

havanagrawal commented Apr 24, 2016 •
edited
Loading

I've used wit/number for the amount, since using wit/amount_of_money was also grabbing the unit, like usd and inr, along with the number.

Again, the tests will fail, so can you please update your side of Wit?
https://wit.ai/havanagrawal/JARVIS-on-Messenger/intents/currency

(P.S. I think we should update this line of the code with the latest date)
r = requests.get('https://api.wit.ai/message?v=20160420&q=' + input, headers={ 'Authorization': 'Bearer %s' % WIT_AI_ACCESS_TOKEN })

(P.P.S For some reason, all the tests have failed... Did you change your Wit access token?)

@swapagarwal
Copy link
Owner

I restarted the build and all tests passed.
I didn't have to update wit as the tests don't capture the new feature (amount).
For date, it's better to keep it as is so that we know it works! (the api may change anytime)
Is there a collaboration feature in wit?

@havanagrawal
Copy link
Contributor Author

havanagrawal commented Apr 24, 2016 •
edited
Loading

Cool.. I've dropped Wit Help a message regarding collaboration.. Hopefully they'll reply within the week.. Anything else I need to do here?

@swapagarwal
Copy link
Owner

swapagarwal commented Apr 24, 2016 •
edited
Loading

Update the tests to include from, to, and amount as well (in the entities returned by the process_query).

@havanagrawal
Copy link
Contributor Author

I've added 2 tests for from_currency, to_currency and number
Could you please (when you get time) add some training data on your side so the tests pass?

You can pick them up from https://wit.ai/havanagrawal/JARVIS-on-Messenger/intents/currency

@swapagarwal swapagarwal merged commit b39b79d into swapagarwal:master Apr 27, 2016
@swapagarwal
Copy link
Owner

Thanks @havanagrawal! 👍

edadesd pushed a commit to edadesd/JARVIS-on-Messenger that referenced this pull request Jul 12, 2017
# This is a combination of 19 commits.
4ad772a 
# This is the 1st commit message:

Added more tests to test_anime

# This is the commit message swapagarwal#2:

Changed Hummingbird API to Kitsu
# This is the commit message swapagarwal#3:

Added bad query test to test_anime

# This is the commit message swapagarwal#4:

Based slice length on len

# This is the commit message swapagarwal#5:

Removed repeated searches.

# This is the commit message swapagarwal#6:

Broke tests into different functions.

# This is the commit message swapagarwal#7:

Removed whitespace.

# This is the commit message swapagarwal#8:

Added tests based on API response and rating format.

# This is the commit message swapagarwal#9:

Reorganized some calls.

# This is the commit message swapagarwal#10:

Refactored API requests, added tests for rank and episode count

# This is the commit message swapagarwal#11:

Added another test case.

# This is the commit message swapagarwal#12:

Added tests for kitsu and youtube links

# This is the commit message swapagarwal#13:

Stickler corrections.

# This is the commit message swapagarwal#14:

More Stickler corrections.

# This is the commit message swapagarwal#15:

More Stickler corrections.

# This is the commit message swapagarwal#16:

Stickler corrections.

# This is the commit message swapagarwal#17:

Moved declarations closer to point used.

# This is the commit message swapagarwal#18:

Style corrections.

# This is the commit message swapagarwal#19:

Style correction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
New Module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@havanagrawal @swapagarwal