-
Notifications
You must be signed in to change notification settings - Fork 322
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
Visual search file upload #635
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok but please see my comments.
. Updated Implementation javascript
// Example usage |
if ('image_file' in allowedParams) { | ||
var imageFileData = handleImageFile(allowedParams.image_file); | ||
return call_api('post', ['resources', 'visual_search'], { image_file: imageFileData }, callback, options); | ||
} | ||
return call_api('get', ['resources', 'visual_search'], allowedParams, callback, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can perform the same post request in both cases
Brief Summary of Changes
or
What Does This PR Address?
Are Tests Included?
Reviewer, Please Note:
I added the
requests
dependency because writing an http request with multi-part file upload using vanilla node is a complicated task and to be honest, I failed at that. I tried writing it from scratch and also reusing some components from the upload.js file but the way this sdk is designed does not allow reusing some of the functions there. At least not in an easy way that won't potentially mean a broken contract. Eventually, I'd prefer doing all http requests using an external library rather than avoiding adding it in the first place - to me, the cost of maintaining low level code is much higher than adding this single dependency.