-
Notifications
You must be signed in to change notification settings - Fork 53
Added the favoriteImage method and tests #122
Added the favoriteImage method and tests #122
Conversation
Merging develop from origin
Merge upstream
In addition to adding the favoriteImage method, this update adds tests for both favoriteImage and deleteImage.
@@ -86,7 +86,11 @@ imgur._imgurRequest = async (operation, payload, extraFormParams) => { | |||
break; | |||
case 'search': | |||
options.method = 'GET'; | |||
options.url += '/gallery/search/' + payload; | |||
options.url += 'gallery/search/' + payload; |
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.
This leading / doesn't break the request but it would break any tests we would want to create for this endpoint. I think this leading slash was left in by mistake, and after spending more time with this code I feel confident that is the case.
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.
i'm curious, in what way do tests break? is it bc of the way the url is matched in the handler?
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.
@kaimallea correct. It would be easy to fix the test to suite, but then the URL would look weird:
https://api.imgur.com/3//gallery/search/lDrXtHj
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.
👍🏾
@@ -105,15 +109,13 @@ imgur._imgurRequest = async (operation, payload, extraFormParams) => { | |||
}; | |||
} | |||
|
|||
if (operation === 'upload' || operation === 'update') { | |||
if (typeof extraFormParams === 'object') { |
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.
Instead of limiting this block of code by the type of operation, let's just add the form params if the extraFormParams parameter is an object. This includes the 'upload' and 'update' operations as well as anything else that would need to use extraFormParams in an imgurRequest.
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.
🎉
@@ -337,7 +339,20 @@ imgur.deleteImage = async (deleteHash) => { | |||
throw new Error('Missing delete hash'); | |||
} | |||
|
|||
return await imgur._imgurRequest('delete', 'deleteHash'); | |||
return await imgur._imgurRequest('delete', deleteHash); |
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.
Had we had tests for this method, we would have caught this obvious error. Now, we do have tests!
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.
wow! tsk tsk @ myself 😞
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR also includes a couple of sneaky fixes.